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

[wael yehia via Phabricator via llvm-commits]

w2yehia added a subscriber: sfertile.
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;
----------------
w2yehia wrote:
> 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?
@nikic I will implement your suggestion. I had to convince myself (and get educated on these flags)

After brainstorming with @sfertile about the implications of the nuw/nsw flags, we concluded that doing only the `&& !OBI->hasNoSignedWrap()` is not a sufficient condition to safely interpret `Scale` (2nd) operand of the `mul` as unsigned in isolation. 
Consider this pseudo-llvm where the `-4` is intended to be signed:
```
if ( %X == 1 || %X == 0) {
  %0 = mul nuw i32 %X, -4   // 1 * 4294967292 = 4294967292 (no unsigned wrap)
  ..
```
However, because this exit condition applies to all visited instructions (add, mul, shl), having the `nuw` means you cannot having something like `24 - 4` because it unsigned overflows. So if we expand the above example:
```
if ( %X == 1 || %X == 0) {
  %0 = mul nuw i32 %X, -4   // 1 * 4294967292 = 4294967292 (no unsigned wrap)
  %1 = add nuw i32 %0, C    // 4294967292 + C
   alloca i8, %1
```
The addition `4294967292 + C` must overflow in order to produce a positive signed number. If it does not overflow then the user-intended result is simply a large unsigned number and storing it in an unsigned is fine.


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

https://reviews.llvm.org/D126546