[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))
----------------
apilipenko wrote:
> 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. 
That seems reasonable, but can you add a comment indicating that it's being conservative and that the example transformation might not happen?


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1413
+        ConstantExpr::getFPToSI(CFP, LHSIntVal->getType());
+        if (LHSConv->hasOneUse() &&
+            ConstantExpr::getSIToFP(CI, I.getType()) == CFP &&
----------------
apilipenko wrote:
> 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.
I see.  I missed that.


================
Comment at: lib/Transforms/InstCombine/InstCombineAddSub.cpp:1414
+        if (LHSConv->hasOneUse() &&
+            ConstantExpr::getSIToFP(CI, I.getType()) == CFP &&
+            WillNotOverflowSignedAdd(LHSIntVal, CI, I)) {
----------------
apilipenko wrote:
> 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.
Yes, that's true.


https://reviews.llvm.org/D31182