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

[cameron.mcinally]

Summary

One incoming comment inline about a tough decision...

[cameron.mcinally]

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]

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?

[cameron.mcinally]

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...

[spatel]

In D62077#1508478, @cameron.mcinally wrote:

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).

[cameron.mcinally]

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.

[spatel]

In D62077#1508771, @cameron.mcinally wrote:

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.

llvm/trunk/lib/Analysis/InstructionSimplify.cpp

if (match(Op1, m_PosZeroFP()))
     return Op0;
 
   // fsub X, -0 ==> X, when we know X is not -0
   if (match(Op1, m_NegZeroFP()) &&
       (FMF.noSignedZeros() || CannotBeNegativeZero(Op0, Q.TLI)))
     return Op0;
 
   // 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;
 
   // fsub 0.0, (fsub 0.0, X) ==> X if signed zeros are ignored.
   // fsub 0.0, (fneg X) ==> X if signed zeros are ignored.
   if (FMF.noSignedZeros() && match(Op0, m_AnyZeroFP()) &&
       (match(Op1, m_FSub(m_AnyZeroFP(), m_Value(X))) ||
        match(Op1, m_FNeg(m_Value(X)))))
     return X;

llvm/trunk/test/Transforms/InstSimplify/floating-point-arithmetic.ll

;
   %t1 = fsub float -0.0, %a
   %ret = fsub float -0.0, %t1
   ret float %ret
 }
 
 ; fsub -0.0, (fneg X) ==> X
 define float @fneg_x(float %a) {
 ; CHECK-LABEL: @fneg_x(
-; CHECK-NEXT:    %t1 = fneg float %a
-; CHECK-NEXT:    %ret = fsub float -0.000000e+00, %t1
-; CHECK-NEXT:    ret float %ret
+; CHECK-NEXT:    ret float [[A:%.*]]
 ;
   %t1 = fneg float %a
   %ret = fsub float -0.0, %t1
   ret float %ret
 }
 
 define <2 x float> @fsub_-0_-0_x_vec(<2 x float> %a) {
 ; CHECK-LABEL: @fsub_-0_-0_x_vec(
 ; CHECK-NEXT:    ret <2 x float> [[A:%.*]]
 ;
   %t1 = fsub <2 x float> <float -0.0, float -0.0>, %a
   %ret = fsub <2 x float> <float -0.0, float -0.0>, %t1
   ret <2 x float> %ret
 }
 
 define <2 x float> @fneg_x_vec(<2 x float> %a) {
 ; CHECK-LABEL: @fneg_x_vec(
-; CHECK-NEXT:    %t1 = fneg <2 x float> %a
-; CHECK-NEXT:    %ret = fsub <2 x float> <float -0.000000e+00, float -0.000000e+00>, %t1
-; CHECK-NEXT:    ret <2 x float> %ret
+; CHECK-NEXT:    ret <2 x float> [[A:%.*]]
 ;
   %t1 = fneg <2 x float> %a
   %ret = fsub <2 x float> <float -0.0, float -0.0>, %t1
   ret <2 x float> %ret
 }
 
 define <2 x float> @fsub_-0_-0_x_vec_undef_elts(<2 x float> %a) {
 ; CHECK-LABEL: @fsub_-0_-0_x_vec_undef_elts(
 ; CHECK-NEXT:    ret <2 x float> [[A:%.*]]
 ;
   %t1 = fsub <2 x float> <float undef, float -0.0>, %a
   %ret = fsub <2 x float> <float -0.0, float undef>, %t1
   ret <2 x float> %ret
 }
 
 define <2 x float> @fneg_x_vec_undef_elts(<2 x float> %a) {
 ; CHECK-LABEL: @fneg_x_vec_undef_elts(
-; CHECK-NEXT:    %t1 = fneg <2 x float> %a
-; CHECK-NEXT:    %ret = fsub <2 x float> <float -0.000000e+00, float undef>, %t1
-; CHECK-NEXT:    ret <2 x float> %ret
+; CHECK-NEXT:    ret <2 x float> [[A:%.*]]
 ;
   %t1 = fneg <2 x float> %a
   %ret = fsub <2 x float> <float -0.0, float undef>, %t1
   ret <2 x float> %ret
 }
 
 ; fsub -0.0, (fsub 0.0, X) != X
 define float @fsub_-0_0_x(float %a) {