[PATCH] D51215: Fix misfolding of IRBuilder.CreateICmp(int_ty X, bitcast (float_ty Y) to int_ty)

[Andrew Scheidecker via Phabricator via llvm-commits]

AndrewScheidecker added inline comments.


================
Comment at: lib/IR/ConstantFold.cpp:1986-1994
     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);
----------------
lebedev.ri wrote:
> And what if the left side is FP? Or both of them?
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D51215