Page MenuHomePhabricator

[InstSimplify] Teach fsub -0.0, (fneg X) ==> X about unary fneg
ClosedPublic

Authored by cameron.mcinally on May 17 2019, 1:10 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2019, 1:10 PM
cameron.mcinally marked an inline comment as done.May 17 2019, 1:15 PM
cameron.mcinally added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4402 ↗(On Diff #200086)

I wasn't sure whether to go with the code above or explicitly match both FNeg and FSub. I.e.

// fsub -0.0, (fsub -0.0, X) ==> X
// fsub -0.0, (fneg X) ==> X
Value *X;
if (match(Op0, m_NegZeroFP()) &&
    (match(Op1, m_FSub(m_NegZeroFP(), m_Value(X))) ||
     match(Op1, m_FNeg(m_Value(X)))))
  return X;

An FSub(-0.0, X) --> FNeg(X) is ok -- and we should probably match them as such, as we do now. But a purist in the future might want to separate the two.

Either option is safe to do, since tests will catch the purist's future change to m_FNeg(...). So it's really just a community preference.

spatel accepted this revision.May 19 2019, 7:43 AM

LGTM

llvm/lib/Analysis/InstructionSimplify.cpp
4402 ↗(On Diff #200086)

The current patch seems fine to me; as you said, the tests future-proof changes to the underlying matcher.

4405 ↗(On Diff #200086)

Semi-independent of this patch, but might want to change that m_AnyZeroFP() to m_ZeroFP() now?

This revision is now accepted and ready to land.May 19 2019, 7:43 AM
This revision was automatically updated to reflect the committed changes.

Semi-independent of this patch, but might want to change that m_AnyZeroFP() to m_ZeroFP() now?

@spatel, I did not make this change with the patch. Did you mean m_PosZeroFP() instead of m_ZeroFP()? Just making sure there's not a new design decision that I don't know about...

Semi-independent of this patch, but might want to change that m_AnyZeroFP() to m_ZeroFP() now?

@spatel, I did not make this change with the patch. Did you mean m_PosZeroFP() instead of m_ZeroFP()? Just making sure there's not a new design decision that I don't know about...

Oops - yes, I meant "m_PosZeroFP()" on the 1st match statement. There's no way that a -0.0 could reach that check if I'm seeing it correctly (this patch took care of that possibility).

Actually, it looks like the sign can stick around regardless of NSZ. This test fails with the proposed change:

define <2 x float> @fsub_0_0_x_vec_undef2(<2 x float> %a) {
; CHECK-LABEL: @fsub_0_0_x_vec_undef2(
; CHECK-NEXT:    ret <2 x float> [[A:%.*]]
;
  %t1 = fsub <2 x float> zeroinitializer, %a
  %ret = fsub nsz <2 x float> <float undef, float -0.0>, %t1
  ret <2 x float> %ret
}

LangRef says:

nsz
No Signed Zeros - Allow optimizations to treat the sign of a zero argument or result as insignificant.

So I don't think we're guaranteed that -0.0 will be consistently converted to +0.0 when NSZ is in play.

Actually, it looks like the sign can stick around regardless of NSZ. This test fails with the proposed change:

define <2 x float> @fsub_0_0_x_vec_undef2(<2 x float> %a) {
; CHECK-LABEL: @fsub_0_0_x_vec_undef2(
; CHECK-NEXT:    ret <2 x float> [[A:%.*]]
;
  %t1 = fsub <2 x float> zeroinitializer, %a
  %ret = fsub nsz <2 x float> <float undef, float -0.0>, %t1
  ret <2 x float> %ret
}

LangRef says:

nsz
No Signed Zeros - Allow optimizations to treat the sign of a zero argument or result as insignificant.

So I don't think we're guaranteed that -0.0 will be consistently converted to +0.0 when NSZ is in play.

Ah, I didn't notice that test; disregard my suggestion.