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

[Artur Pilipenko via Phabricator via llvm-commits]

apilipenko 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))
----------------
andrew.w.kaylor wrote:
> 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.
Given that we are uncertain if we need this transform at all (see the discussion above) I'd prefer not to make it too smart without a good reason. So, unless we have a clear reason why we want this code to be more aggressive I'd prefer to keep this fix as simple as it is now.

For more aggressive version of this transform we should rely on Float2Int pass and make it smarter if needed. 


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1413
+        ConstantExpr::getFPToSI(CFP, LHSIntVal->getType());
+        if (LHSConv->hasOneUse() &&
+            ConstantExpr::getSIToFP(CI, I.getType()) == CFP &&
----------------
andrew.w.kaylor wrote:
> This condition will block either transformation and so it could be checked at line 1395 and save us a bit of work sometimes.
The second transform looks for either RHSConv or LHSConv to have one use, so it doesn't always block the second transform.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1414
+        if (LHSConv->hasOneUse() &&
+            ConstantExpr::getSIToFP(CI, I.getType()) == CFP &&
+            WillNotOverflowSignedAdd(LHSIntVal, CI, I)) {
----------------
andrew.w.kaylor wrote:
> It looks like getFPToSI and getSIToFP will end up using the APFloat class.  Wouldn't it be better to just use APFloat here and call APFloat::isInteger()?
It sounds like a separate refactoring to the existing code.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1429
+        // single use (so we don't increase the number of int->fp conversions),
+        // and if the integer add will not overflow.
+        if (LHSIntVal->getType() == RHSIntVal->getType() &&
----------------
scanon wrote:
> Aren't we missing a check that RHSIntVal can be exactly in the floating-point type here?  I.e. `if (RHSIntVal->getType()->getIntegerBitWidth() <= MaxRepresentableBits)`, mirroring the check for LHS above?
We should be fine as is. I assume that the LHS, RHS of the addition have the same types (there can be an assert) and there is also a check below that integer types are the same.


================
Comment at: test/Transforms/InstCombine/add-sitofp.ll:16
+; CHECK: fadd float
+  %m = lshr i32 %a, 24
+  %n = and i32 %m, %b
----------------
boris.ulasevich wrote:
> Why do we need lshr here? %n + 1 = ((%m >>> 24) && %b) + 1 takes range 1..256 - it fits quite well to 23-bit single precision mantissa, and optimisation can be applied here, isn't it?
> Does it make sense for you? Do I miss something?
I just duplicated the example above. I agree it is confusing in this context. I'll simplify the negative test case to make it more clear that the result of the operation is not guaranteed to fit into float type.


https://reviews.llvm.org/D31182