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

[Andy Kaylor via Phabricator via llvm-commits]

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?


https://reviews.llvm.org/D31182