[llvm-commits] [llvm] r132926 - in /llvm/trunk: lib/Transforms/InstCombine/InstCombineCasts.cpp test/Transforms/InstCombine/2011-06-13-nsw-alloca.ll

[John McCall]

On Jun 14, 2011, at 8:46 PM, Stuart Hastings wrote:

> 
> On Jun 13, 2011, at 12:57 PM, Duncan Sands wrote:
> 
>> Hi Stuart,
>> 
>>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp (original)
>>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineCasts.cpp Mon Jun 13 13:48:49 2011
>>> @@ -71,6 +71,11 @@
>>>   // This requires TargetData to get the alloca alignment and size information.
>>>   if (!TD) return 0;
>>> 
>>> +  // Insist that the amount-to-allocate not overflow.
>>> +  OverflowingBinaryOperator *OBI =
>>> +    dyn_cast<OverflowingBinaryOperator>(AI.getOperand(0));
>>> +  if (OBI&&  !(OBI->hasNoSignedWrap() || OBI->hasNoUnsignedWrap())) return 0;
>> 
>> is it correct to allow both nsw and nuw here?  Is the amount signed or unsigned?
> 
> Sort of.  Clang actually generates nsw for some alloca parameters, but that doesn't make sense to me; alloca properly takes an unsigned quantity.
> 
> Accepting nsw and nuw here allows InstCombine to do this transformation safely; both indicate no wraparound.  Before my patch, InstCombine didn't check for either of these, and we found a test case that would break.
> 
> I'll take this up with the Clang folks.

Okay, so as I understand it, we're doing an analysis which tries to convert alloca i8, %v into alloca T, %v'.  In order for this to be sound, we have to prove that %v is an exact multiple of sizeof(T) and that %v' * sizeof(T) is %v without unsigned overflow — it's this "without overflow" condition that we're missing, which means that we can turn valid input (because it's computed with a defined overflow), into undefined behavior.

Given that, the conservatively correct thing is to require all the scaling arithmetic that we're going to remove or adjust to be nuw.  Since C does not have nuw operations, it's unclear that this optimization will ever fire in practice.

We do have an extra constraint here:  it is already undefined behavior if %v is "obviously too big", as it would be if it were (say) negative when interpreted as signed.  My intuition is that, working backwards through %v, there are circumstances under which we could recognize that nsw arithmetic is sufficient.  For example, if
  %v = mul nsw i64 %x, sizeof T
then the nsw, together with the guarantee that %v is positive, should be sufficient to rewrite this with %v' := %x.  I don't know how far down the rabbit hole it's worth pursuing this — the analysis gets complicated real quick, and this transform is only useful if it actually enables or simplifies downstream optimization, since it has no direct impact on the quality of the final machine code.  It's also a pretty marginal case, since frontends should be emitting VLAs with appropriately-typed allocas in the first place, and so this pattern of code will probably only arise with actual calls to alloca() (or VLAs that the user writes as char[] for some reason).

John.