[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
Thu Mar 23 15:32:21 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))
----------------
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
More information about the llvm-commits
mailing list