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

[Chris Lattner]

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.

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

> 
>> 
>> +  
>> +  // If the strides are equal, then this is just a (complex) loop invariant
>> +  // comparison of a/b.
>> +  if (LHSStride == RHSStride)
>> +    return SE.getMinusSCEV(LHSA->getStart(), RHSA->getStart());
> 
> The comment should say a-b, not a/b.

Ok, I didn't actually mean divide, I tweaked the wording.

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

Thanks for the review!!

-Chris