[PATCH] D126546: [InstCombine] decomposeSimpleLinearExpr should bail out on negative operands.

[wael yehia via Phabricator via llvm-commits]

w2yehia added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:40
     OverflowingBinaryOperator *OBI = dyn_cast<OverflowingBinaryOperator>(Val);
     if (OBI && !OBI->hasNoUnsignedWrap() && !OBI->hasNoSignedWrap()) {
       Scale = 1;
----------------
nikic wrote:
> w2yehia wrote:
> > nikic wrote:
> > > Isn't the actual bug here? The `&& !OBI->hasNoSignedWrap()` shouldn't be there. The number of elements is an unsigned value, so we should be checking for no unsigned wrap, not "either no unsigned or no signed wrap".
> > i'm not that familiar with the implications of (not) having `nsw`, so you are likely correct.
> > Does the presence of `nsw` indicate the operands of the multiplication are signed?
> nsw indicates that there is no signed overflow, while we need no unsigned overflow here.
Thanks for reviewing. I had to read up more about nsw/nuw and signedness in llvm in general.
I couldn't conclude that the presence of `nuw` indicates that the operands of the mul can be safely interpreted as unsigned.
I might be asking the wrong question, so can you please tell me why checking for `nuw` is necessary and sufficient in order to make the code (in decomposeSimpleLinearExpr and PromoteCastOfAllocation) to be sound?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126546/new/

https://reviews.llvm.org/D126546