Fix Reassociate handling of constant in presence of undef float

[mehdi_amini]

Summary

Just implemented the patch you suggested along with the test case.

[spatel]

test/Transforms/Reassociate/undef.ll

4 ↗

(On Diff #18216)

Please use FileCheck for the expected output rather than just assuming a lack of crash is correct.

Nit: "The Reassociate pass used to crash on these examples."

[mehdi_amini]

test/Transforms/Reassociate/undef.ll

4 ↗

(On Diff #18216)

Why should I add a Filecheck for a crash? I looked at what was done by other crash test in the test suite.
What should I check exactly?

Thanks!

[spatel]

test/Transforms/Reassociate/undef.ll

4 ↗

(On Diff #18216)

It's just improved testing. If you're going to add a test (which of course you have to), then you might as well make that test check for correctness rather than absence of incorrectness.

I realize that there are existing tests that violate this principle. They can and should be improved when possible.

In this case, you want something like (I didn't actually run this):
CHECK-LABEL: @undef1(
CHECK-NEXT: wrapper_entry:
CHECK-NEXT: ret float fadd

CHECK-LABEL: @undef2(
CHECK-NEXT: wrapper_entry:
CHECK-NEXT: unreachable

[mehdi_amini]

Update taking into account the review

(even though I'm not convince that sometime you *can* test something. Here for the first test the result is

ret float fadd (float undef, float fadd (float undef, float fadd (float fsub (float -0.000000e+00, float undef), float fsub (float -0.000000e+00, float undef))))

if someone improves Reassociate to fold undef that is equally fine in my opinion and we are just adding some maintenance burden/noise to update test.)

[majnemer]

LGTM

[mehdi_amini]

r226245

[spatel]

In D6993#109628, @joker.eph wrote:

(even though I'm not convince that sometime you *can* test something. Here for the first test the result is

ret float fadd (float undef, float fadd (float undef, float fadd (float fsub (float -0.000000e+00, float undef), float fsub (float -0.000000e+00, float undef))))

if someone improves Reassociate to fold undef that is equally fine in my opinion and we are just adding some maintenance burden/noise to update test.)

That's true, but:

1. You don't have to fully specify each line of IR in your CHECKs.
2. You didn't create the minimal test case to check your patch. I haven't looked into how this crashes, but this is certainly a smaller test case that still crashes: define float @undef1(float %x) { %y = fsub fast float undef, %x %z = fsub fast float undef, %y ret float %z }

lib/Transforms/Scalar/Reassociate.cpp

for (unsigned i = 0, e = NodesToRewrite.size(); i != e; ++i)
     RedoInsts.insert(NodesToRewrite[i]);
 }
 
 /// NegateValue - Insert instructions before the instruction pointed to by BI,
 /// that computes the negative version of the value specified.  The negative
 /// version of the value is returned, and BI is left pointing at the instruction
 /// that should be processed next by the reassociation pass.
 static Value *NegateValue(Value *V, Instruction *BI) {
-  if (ConstantFP *C = dyn_cast<ConstantFP>(V))
+  if (Constant *C = dyn_cast<Constant>(V)) {
+    if (C->getType()->isFPOrFPVectorTy()) {
       return ConstantExpr::getFNeg(C);
-  if (Constant *C = dyn_cast<Constant>(V))
+    }
     return ConstantExpr::getNeg(C);
+  }
+
 
   // We are trying to expose opportunity for reassociation.  One of the things
   // that we want to do to achieve this is to push a negation as deep into an
   // expression chain as possible, to expose the add instructions.  In practice,
   // this means that we turn this:
   //   X = -(A+12+C+D)   into    X = -A + -12 + -C + -D = -12 + -A + -C + -D
   // so that later, a: Y = 12+X could get reassociated with the -12 to eliminate
   // the constants.  We assume that instcombine will clean up the mess later if

test/Transforms/Reassociate/crash2.ll

+; RUN: opt -reassociate %s -S -o - | FileCheck %s
+
+; Reassociate pass used to crash on these example
+
+
+define float @undef1() {
+wrapper_entry:
+; CHECK-LABEL: @undef1
+; CHECK: ret float fadd (float undef, float fadd (float undef, float fadd (float fsub (float -0.000000e+00, float undef), float fsub (float -0.000000e+00, float undef))))
+  %0 = fadd fast float undef, undef
+  %1 = fsub fast float undef, %0
+  %2 = fadd fast float undef, %1
+  ret float %2
+}
+
+define void @undef2() {
+wrapper_entry:
+; CHECK-LABEL: @undef2
+; CHECK: unreachable
+  %0 = fadd fast float undef, undef
+  %1 = fadd fast float %0, 1.000000e+00
+  %2 = fsub fast float %0, %1
+  %3 = fmul fast float %2, 2.000000e+00
+  unreachable
+}