[Reassoc] Small fix to support unary FNeg in NegateValue(...)

[cameron.mcinally]

Summary

Here's a small patch to fix an assert in NegateValue(...).

This may need further performance improvements...

[mcberg2017]

LGTM, but I will give Sanjay some time before moving forward.

llvm/lib/Transforms/Scalar/Reassociate.cpp

916

You will need to revise this one too in D61675 as it will be a unary then. Just a note to remember.

[mcberg2017]

BTW, general comment: Many of the remaining BinaryOperator contexts in Reassociate.cpp will need re-examaination too as UnaryOperator may also be present in places where it currently is not.

[cameron.mcinally]

llvm/lib/Transforms/Scalar/Reassociate.cpp

916

I think this one is okay. This is creating a new binary FNeg.

I'm still working out the details, but I suspect that allowing Reassociation to form binary FNegs is good (i.e. better trees). E.g.:

x + -y

can be safely transformed to:

x + (-0.0 - y)

as long as the x isn't optimized away (Sanjay showed this in another Diff). I can elaborate if anyone is interested...

[cameron.mcinally]

In D66612#1641702, @mcberg2017 wrote:

BTW, general comment: Many of the remaining BinaryOperator contexts in Reassociate.cpp will need re-examaination too as UnaryOperator may also be present in places where it currently is not.

Agreed.

If you have any test cases that I can use as an intuition pump, that would be great. I haven't worked that much in Reassociate.

[spatel]

LGTM

llvm/lib/Transforms/Scalar/Reassociate.cpp

916

For reference, the reasoning is that if 'y' is NaN, and it's subsequently used by the fadd, we aren't required to preserve the resulting signbit of that NaN through the fadd op.

llvm/test/Transforms/Reassociate/2019-08-22-FNegAssert.ll

1

I think it's better to use the usual script here and generate the expected output. In other words, if we're going to test this code for failure, we might as well test it for correctness at the same time.

The extra checking may also reveal unintended behavior for future patches sooner rather than later.

llvm/lib/Transforms/Scalar/Reassociate.cpp

static Value *NegateValue(Value *V, Instruction *BI,
   for (User *U : V->users()) {
     if (!match(U, m_Neg(m_Value())) && !match(U, m_FNeg(m_Value())))
       continue;
 
     // We found one!  Now we have to make sure that the definition dominates
     // this use.  We do this by moving it to the entry block (if it is a
     // non-instruction value) or right after the definition.  These negates will
     // be zapped by reassociate later, so we don't need much finesse here.
-    BinaryOperator *TheNeg = cast<BinaryOperator>(U);
+    Instruction *TheNeg = cast<Instruction>(U);
 
     // Verify that the negate is in this function, V might be a constant expr.
     if (TheNeg->getParent()->getParent() != BI->getParent()->getParent())
       continue;
 
     bool FoundCatchSwitch = false;
 
     BasicBlock::iterator InsertPt;
if (TheNeg->getOpcode() == Instruction::Sub) {
       TheNeg->andIRFlags(BI);
     }
     ToRedo.insert(TheNeg);
     return TheNeg;
   }
 
   // Insert a 'neg' instruction that subtracts the value from zero to get the
   // negation.
   BinaryOperator *NewNeg = CreateNeg(V, V->getName() + ".neg", BI, BI);
   ToRedo.insert(NewNeg);
   return NewNeg;
 }
 
 /// Return true if we should break up this subtract of X-Y into (X + -Y).
 static bool ShouldBreakUpSubtract(Instruction *Sub) {
   // If this is a negation, we can't split it up!
   if (match(Sub, m_Neg(m_Value())) || match(Sub, m_FNeg(m_Value()))) 

llvm/test/Transforms/Reassociate/2019-08-22-FNegAssert.ll

+; RUN: opt < %s -reassociate -disable-output
+; D61675
+
+define float @test1(float %arg, float %arg1, float %arg2, float %arg3) {
+  %tmp1 = fsub fast float %arg3, %arg2
+  %tmp2 = fadd fast float %arg, -5.000000e+02
+  %tmp3 = fsub fast float %tmp1, %tmp2
+  %tmp4 = fmul fast float %tmp3, %tmp3
+  %tmp5 = fneg fast float %arg
+  %tmp6 = fdiv fast float %tmp5, %arg1
+  %tmp7 = fadd fast float %tmp6, %tmp4
+  ret float %tmp7
+}