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

[Dan Gohman]

On Jan 9, 2011, at 2:26 PM, Chris Lattner wrote:
> @@ -2448,22 +2448,21 @@
> 
> /// 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.

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

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

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

Dan