[llvm-commits] InstCombining, small bug alloca + bitcast

[Duncan Sands]

Hi Stepan,

> Look at string:
> if (OBI && !OBI->hasNoUnsignedWrap())
> That's original string that was before my patch.

yes, I know.  Whoever did that should have added comments to *the function
description* explaining what semantics the function provides.  While that
isn't your fault, it would be great if you could fix this.

>
> I had replaced it with
> if (OBI && !OBI->hasNoUnsignedWrap() && !OBI->hasNoSignedWrap())
>
> You think that this string really should be commented?

No, I think that the function description should explain what it is really
doing, since it is clearly doing more than the comment describes.  It is
providing some kind of overflow guarantees.  What are they?

> As I understood, here I supposed that nuw and nsw gives guarantee that value
> will not exceed MAX_INT and MAX_UINT respectively, since in another case it will
> poison value. And the value also will not wrapped from MAX_UINT to 0. So it is
> assumed that result will always linearly depended from arguments. It means that
> we can reduce case
> %7 = add nsw i32 %6, 2048
> %8 = alloca i8, i32 %7
> %9 = bitcast i8* %8 to double*
>
> to the single "alloca double, i32 %recalculated_size " instruction.

Thanks for explaining, however this doesn't make any sense to me.  Is the alloca
amount signed or unsigned?  While LangRef doesn't say this, as allocating -1
bytes doesn't make any sense, it seems clear that the amount is unsigned.  What
the transform is doing is it considers (alloca_amount udiv 8) and sees if this
simplifies, eliminating the udiv, with no remainder.  If so it forms the new
alloca using the division result for the size.

Your nsw change is basically saying: if (X udiv N) simplifies with no remainder
and (Y udiv N) simplifies with no remainder, then (X +nsw Y) udiv N
simplifies with no remainder.  Here is a counterexample.  I will work in i8 for
simplicity.

Take N = 3 (size of alloca'd type).  Take X = 255 and Y = 3.  Then X udiv 3 = 85 
with no remainder.  Also Y udiv 3 = 1 with no remainder.  As a signed number
X = -1 and Y = 3, thus X + Y does not overflow as a signed computation, i.e.
this can be written as X +nsw Y.  However X + Y = 2, and 2 udiv 3 has a
remainder.

Here's an alternative approach: delete DecomposeSimpleLinearExpr and instead
call SimplifyURemInst with alloca_amount as left-hand side and the size (N) as
right-hand side.  If this doesn't simplify, or simplifies to something non-zero
then bail out.  Otherwise (it simplified to zero) call SimplifyUDivInst with
the same left and right hand sides.  If it failed to simplify then bail out.
Otherwise use the simplified value as the new alloca size.

The advantage of using InstructionSimplify is that it is much more widely used
than DecomposeSimpleLinearExpr.  Thus it is both better tested and any
improvements to it will benefit many places.  Maybe DecomposeSimpleLinearExpr
knows a trick or two that InstructionSimplify does not.  If so, then you should
teach InstructionSimplify those tricks and lots of code will benefit.  In short
I think the right thing to do is to delete DecomposeSimpleLinearExpr and use
the InstructionSimplify infrastructure instead.

Ciao, Duncan.

>
> The same with "nuw" attr.
>
> So I just added case for nsw, since probably author of original string forgot to
> do it.
>
> -Stepan.
>
> Duncan Sands wrote:
>> Hi Stepan,
>>
>>> I also added some usefull checks to test and fixed InstCombineCasts.cpp.
>>
>> what was wrong with InstCombineCasts? In fact the existing check
>>
>> // Cannot look past anything that might overflow.
>> OverflowingBinaryOperator *OBI = dyn_cast<OverflowingBinaryOperator>(Val);
>> if (OBI&& !OBI->hasNoUnsignedWrap()) {
>> Scale = 1;
>> Offset = 0;
>> return Val;
>> }
>>
>> looks bogus to me: without it, you correctly get Scale and Offset such that
>> Val = X*Scale + Offset, which is what the routine is supposed to do according
>> to the comment describing it.
>>
>> So if this check is really needed, it must be because of some undocumented
>> semantics in how the routine is supposed to behave. What are these semantics?
>> They should be documented. Once it is clear what this routine is supposed to
>> be doing, it should become clear whether your change is correct or not.
>>
>> Ciao, Duncan.
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>