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

Chris Lattner sabre at nondot.org
Sat Jan 15 18:44:02 PST 2011


On Jan 11, 2011, at 11:36 AM, Dan Gohman wrote:

> 
> 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.

Hrm, I think I see what you mean.  The code in getMinusSCEVForExitTest ends up producing an AddRec from the getMinusExpr (it *knows* it will be an addrec) that counts down to (but doesn't pass) zero.  Do you think a better way to handle this is to just call getMinusSCEV, get the addrec, then ask for the same addrec in non-overflowing form?

>>>> +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.

I don't think that matters for this code.  The code is carefully set up so that it has an AddRec that doesn't overflow "somehow", and it knows which direction it is stepping, and it knows that it gets to a specific integer value.  If it "strides past" this value, the only way for the program to be defined is if it wraps all the way back around to the value.  Just knowing that the addrec has cannot wrap *somewhere* should be enough, we're not doing an analysis in its original domain.

>>>> +  // 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.

Maybe (probably) I'm seriously misunderstanding something, but in this case, we'll end up getting a new SCEV {X+4,+,-2}.  Setting HasNUW on is resultant addrec tells the clients that the *result of the addrec* (not all the subcomputations that happen) will not wrap past zero. This allows the client to know that the crossover point happens at x/2+2.

-Chris



More information about the llvm-commits mailing list