Fix misfolding of IRBuilder.CreateICmp(int_ty X, bitcast (float_ty Y) to int_ty)

[AndrewScheidecker]

Summary

This code was incorrectly folding:

IRBuilder.CreateICmp(int_ty X, bitcast (float_ty Y) to int_ty)

to:

IRBuilder.CreateICmp(bitcast (int_ty X) to float_ty, float_ty Y)

which is malformed IR due to the mismatch between the integer predicate and the FP operands. In my case, this triggered a crash in an optimization pass.

[AndrewScheidecker]

Ping.

[lebedev.ri]

lib/IR/ConstantFold.cpp

1985–1993

And what if the left side is FP? Or both of them?

[AndrewScheidecker]

lib/IR/ConstantFold.cpp

1985–1993

That's a good question. That final else of the function seems to assume that it's folding an icmp (e.g. note the ConstantExpr::getICmp just below), but that is implicit in the control flow.

This branch handles the case where either operand is undef: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1714

This branch handles the case where both operands are constant floats: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1789

This branch handles the case where the operands are vectors of any type: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1832

And this branch handles the case where one or both the operands are floating-point ConstantExprs: https://github.com/llvm-mirror/llvm/blob/71589ff6720f9667b245f0cdb8a26f491c51e9fc/lib/IR/ConstantFold.cpp#L1850

So it should only be possible to reach that final else when the operands are scalar non-FP, assuming that there isn't a way to construct a Constant that has scalar FP type but isn't a ConstantFP or ConstantExpr.

I added assertions locally to verify that this if statement is only ever reached with an ICMP predicate, and with both operands having non-FP type, and they weren't triggered by "make check".

The ConstantExpr::getICmp call inside the if-then clause asserts if pred isn't an icmp predicate, and since an icmp with FP operands is invalid, that implies the original code would not handle a comparison with FP operands that got to this if statement.

[AndrewScheidecker]

I made the test a little simpler by bitcasting a GlobalVariable pointer to a float to get a FP typed ConstantExpr, instead of using a bitcast vector.

[AndrewScheidecker]

Ping.

[spatel]

This took some work, but I have an IR test case that should show the bug:
rL353883

I haven't looked at D51216 yet, so this might be a dumb question, but is there an fcmp sibling to this bug?

unittests/IR/IRBuilderTest.cpp

679 ↗

(On Diff #166177)

This means that IRBuilder has a bug/opportunity to be stricter? How did we create an integer add with FP operands?

[AndrewScheidecker]

This took some work, but I have an IR test case that should show the bug:
rL353883

Nice!

I haven't looked at D51216 yet, so this might be a dumb question, but is there an fcmp sibling to this bug?

There doesn't seem to be. I wrote a fcmp equivalent to the test you added to bitcast.ll in rL353883, and it behaved as expected.

unittests/IR/IRBuilderTest.cpp

679 ↗

(On Diff #166177)

Oops, that should be CreateFAdd. It looks like this would trigger an assertion on a NDEBUG build. But should I keep this test if it's redundant with the IR test case you added?

[spatel]

unittests/IR/IRBuilderTest.cpp

679 ↗

(On Diff #166177)

No - we can remove this test now. Please rebase/update with the new IR test.

[AndrewScheidecker]

Rebased, removed the superfluous IRBuilder test, updated the new IR test to reflect the fix, and added an IR test of the equivalent fcmp misfolding.

[spatel]

LGTM

lib/IR/ConstantFold.cpp

if (C1->getType()->isFloatingPointTy() &&
     }
 
     // If we evaluated the result, return it now.
     if (Result != -1)
       return ConstantInt::get(ResultTy, Result);
 
     // If the right hand side is a bitcast, try using its inverse to simplify
     // it by moving it to the left hand side.  We can't do this if it would turn
-    // a vector compare into a scalar compare or visa versa.
+    // a vector compare into a scalar compare or visa versa, or if it would turn
+    // the operands into FP values.
     if (ConstantExpr *CE2 = dyn_cast<ConstantExpr>(C2)) {
       Constant *CE2Op0 = CE2->getOperand(0);
       if (CE2->getOpcode() == Instruction::BitCast &&
-          CE2->getType()->isVectorTy() == CE2Op0->getType()->isVectorTy()) {
+          CE2->getType()->isVectorTy() == CE2Op0->getType()->isVectorTy() &&
+          !CE2Op0->getType()->isFPOrFPVectorTy()) {
         Constant *Inverse = ConstantExpr::getBitCast(C1, CE2Op0->getType());
         return ConstantExpr::getICmp(pred, Inverse, CE2Op0);
       }
     }
 
     // If the left hand side is an extension, try eliminating it.
     if (ConstantExpr *CE1 = dyn_cast<ConstantExpr>(C1)) {
       if ((CE1->getOpcode() == Instruction::SExt &&
            ICmpInst::isSigned((ICmpInst::Predicate)pred)) ||
           (CE1->getOpcode() == Instruction::ZExt &&
            !ICmpInst::isSigned((ICmpInst::Predicate)pred))){
         Constant *CE1Op0 = CE1->getOperand(0);

test/Transforms/ConstProp/bitcast.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -constprop -S | FileCheck %s
 ; PR2165
 
 define <1 x i64> @test1() {
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT:    ret <1 x i64> <i64 63>
 ;
   %A = bitcast i64 63 to <1 x i64>
   ret <1 x i64> %A
 }
 
-; FIXME: Don't try to propagate an FP source operand to an icmp.
+; Ensure that a FP source operand isn't propagated to an icmp.
 
 @a = external global i16, align 1
 @b = external global i16, align 1
 
 define i1 @bad_icmp_constexpr_bitcast() {
 ; CHECK-LABEL: @bad_icmp_constexpr_bitcast(
-; CHECK-NEXT:    ret i1 icmp eq (float bitcast (i32 ptrtoint (i16* @a to i32) to float), float fadd (float bitcast (i32 ptrtoint (i16* @b to i32) to float), float 2.000000e+00))
+; CHECK-NEXT:    ret i1 icmp eq (i32 ptrtoint (i16* @a to i32), i32 bitcast (float fadd (float bitcast (i32 ptrtoint (i16* @b to i32) to float), float 2.000000e+00) to i32))
 ;
   %cmp = icmp eq i32 ptrtoint (i16* @a to i32), bitcast (float fadd (float bitcast (i32 ptrtoint (i16* @b to i32) to float), float 2.0) to i32)
   ret i1 %cmp
 }
+
+; Ensure that an integer source operand isn't propagated to a fcmp.
+
+@c = external global i16, align 1
+@d = external global i16, align 1
+
+define i1 @bad_fcmp_constexpr_bitcast() {
+; CHECK-LABEL: @bad_fcmp_constexpr_bitcast(
+; CHECK-NEXT:    ret i1 fcmp oeq (float bitcast (i32 ptrtoint (i16* @c to i32) to float), float bitcast (i32 add (i32 ptrtoint (i16* @d to i32), i32 2) to float))
+;
+  %cmp = fcmp oeq float bitcast (i32 ptrtoint (i16* @c to i32) to float), bitcast (i32 add (i32 ptrtoint (i16* @d to i32), i32 2) to float)
+  ret i1 %cmp
+}