[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;
----------------
w2yehia wrote:
> 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.
basically, if you have a complex expression involving add/mul/shl and one of the operands is negative, then in order to bring back the result to a positive value an unsigned overflow has to occur:
1) add i32 -1, C  // (unsigned) -1 + C must overflow
2) mul i32 -1, C  // (unsigned) -1 * C must overflow
3) shl i32 -1, C   // (unsigned) -1 << C must overflow


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

https://reviews.llvm.org/D126546