[PATCH] D31182: [InstCombine] fadd double (sitofp x), y check that the promotion is valid

Andy Kaylor via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 21 13:00:11 PDT 2017

andrew.w.kaylor added inline comments.

Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1404
+      = APFloat::semanticsPrecision(FPType->getFltSemantics());
+    if (LHSIntVal->getType()->getIntegerBitWidth() <= MaxRepresentableBits) {
+      // (fadd double (sitofp x), fpcst) --> (sitofp (add int x, intcst))
This check seems more conservative than we'd want it to be.  For instance, in the example in the comments we know that 'x & 1234' is can be represented as a 12-bit signed integer, but if x is i32 this code will return 32 for LHSIntVal->getType()->getIntegerBitWidth().  This directly relates to Boris' comment on the test case.

Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1410
+      // instcombined.
+      if (ConstantFP *CFP = dyn_cast<ConstantFP>(RHS)) {
+        Constant *CI =
The MaxRepresentableBits calculation and comparison won't be necessary if neither this condition nor the condition at line 1424 is true.  I think the code should be organized to check these dyn_cast conditions first, particularly if you are going to do something more complicated with the number of bits required.

Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1413
+        ConstantExpr::getFPToSI(CFP, LHSIntVal->getType());
+        if (LHSConv->hasOneUse() &&
+            ConstantExpr::getSIToFP(CI, I.getType()) == CFP &&
This condition will block either transformation and so it could be checked at line 1395 and save us a bit of work sometimes.

Comment at: test/Transforms/InstCombine/add-sitofp.ll:21
+  ret float %p
Can you add a negative test for the '(fadd double (sitofp x), (sitofp y)) --> (sitofp (add int x, y))' case also?


More information about the llvm-commits mailing list