[llvm-commits] [llvm] r123131 - in /llvm/trunk: include/llvm/Analysis/ScalarEvolution.h lib/Analysis/ScalarEvolution.cpp

[Dan Gohman]

On Jan 11, 2011, at 9:16 AM, Chris Lattner wrote:

> On Jan 10, 2011, at 2:07 PM, Dan Gohman wrote:
>> On Jan 9, 2011, at 2:26 PM, Chris Lattner wrote:
>>> /// getMinusSCEV - Return a SCEV corresponding to LHS - RHS.
>>> ///
>>> -const SCEV *ScalarEvolution::getMinusSCEV(const SCEV *LHS,
>>> -                                          const SCEV *RHS) {
>>> +const SCEV *ScalarEvolution::getMinusSCEV(const SCEV *LHS, const SCEV *RHS,
>>> +                                          bool HasNUW, bool HasNSW) {
>>> // Fast path: X - X --> 0.
>>> if (LHS == RHS)
>>>   return getConstant(LHS->getType(), 0);
>>> 
>>> // X - Y --> X + -Y
>>> -  return getAddExpr(LHS, getNegativeSCEV(RHS));
>>> +  return getAddExpr(LHS, getNegativeSCEV(RHS), HasNUW, HasNSW);
>> 
>> This isn't safe. For example, suppose X and Y are bot INT_MIN (dynamically).
>> Then X - Y does not overflow, but X + -Y does.
> 
> Ok, the only current client of this is reasoning about the generated add, not the hypothetical subtract.  You're right that this is confusing, I added a clarifying comment in r123243.

For the unsigned case, this is still really confusing. Take the most innocent-
looking expression like "X - 1" for example. This turns into X + UINT_MAX,
which has unsigned overflow.

For the signed case, setting HasNSW requires proving that the RHS is not
UINT_MIN.

A side note here is that the nsw and nuw concepts don't cover every
interesting kind of overflow. There are many situations where one operand
of an add is unsigned and the other operand is signed, but these are hard
to represent since they're asymmetric.

> 
>>> +static const SCEVAddRecExpr *
>>> +isSimpleUnwrappingAddRec(const SCEV *S, const Loop *L) {
>>> +  const SCEVAddRecExpr *SA = dyn_cast<SCEVAddRecExpr>(S);
>>> +  
>>> +  // The SCEV must be an addrec of this loop.
>>> +  if (!SA || SA->getLoop() != L || !SA->isAffine())
>>> +    return 0;
>>> +  
>>> +  // The SCEV must be known to not wrap in some way to be interesting.
>>> +  if (!SA->hasNoUnsignedWrap() && !SA->hasNoSignedWrap())
>>> +    return 0;
>> 
>> It's suspicious that this code doesn't care which of nsw or nuw
>> the instruction has.
> 
> NSW is enough to know that the bounds is limited to (e.g.) 2^32, just in a different coordinate space.   It means you can't "stride past" a value.  

Unsigned overflow happens in the middle of the signed range.
Signed overflow happens at two points along the unsigned range.
It's quite possible to have a well-behaved stride pattern in
one range that doesn't "stride past" its destination, but does
happen to cross over the overflow point of the other range.

>>> +  // If the signs of the strides differ, then the negative stride is counting
>>> +  // down to the positive stride.
>>> +  if (LHSStride->getValue().isNegative() != RHSStride->getValue().isNegative()){
>>> +    if (RHSStride->getValue().isNegative())
>>> +      std::swap(LHS, RHS);
>>> +  } else {
>>> +    // If LHS's stride is smaller than RHS's stride, then "b" must be less than
>>> +    // "a" and "b" is RHS is counting up (catching up) to LHS.  This is true
>>> +    // whether the strides are positive or negative.
>>> +    if (RHSStride->getValue().slt(LHSStride->getValue()))
>>> +      std::swap(LHS, RHS);
>>> +  }
>>> +    
>>> +  return SE.getMinusSCEV(LHS, RHS, true /*HasNUW*/);
>> 
>> This isn't safe if the addrecs are nsw but not nuw (following the suspicion
>> above).
> 
> How so?


Suppose LHS is {X,+,-1}<L> and RHS is {-4,+,1}<L>. Both could legitimately have
NSW and not NUW. In the sixth iteration of the loop, LHS is X-6 and RHS is 1.

X-6 - 1 => X-6 + -1 => X-6 + UINT_MAX, which has unsigned overflow if X != 6.

Dan