[llvm-commits] [llvm] r53508 - in /llvm/trunk: lib/Analysis/ScalarEvolution.cpp test/Analysis/ScalarEvolution/2007-08-06-Unsigned.ll test/Analysis/ScalarEvolution/2008-07-12-UnneededSelect1.ll test/Analysis/ScalarEvolution/2008-07-12-UnneededSelect2.ll

[Dan Gohman]

On Jul 12, 2008, at 12:41 AM, Nick Lewycky wrote:

> Author: nicholas
> Date: Sat Jul 12 02:41:32 2008
> New Revision: 53508
>
> URL: http://llvm.org/viewvc/llvm-project?rev=53508&view=rev
> Log:
> Stop creating extraneous smax/umax in SCEV. This removes a  
> regression where we
> started complicating many loops ('for' loops, in fact).

Cool!

>
>
> Added:
>    llvm/trunk/test/Analysis/ScalarEvolution/2008-07-12- 
> UnneededSelect1.ll
>    llvm/trunk/test/Analysis/ScalarEvolution/2008-07-12- 
> UnneededSelect2.ll
> Modified:
>    llvm/trunk/lib/Analysis/ScalarEvolution.cpp
>    llvm/trunk/test/Analysis/ScalarEvolution/2007-08-06-Unsigned.ll
>
> Modified: llvm/trunk/lib/Analysis/ScalarEvolution.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ScalarEvolution.cpp?rev=53508&r1=53507&r2=53508&view=diff
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Analysis/ScalarEvolution.cpp (original)
> +++ llvm/trunk/lib/Analysis/ScalarEvolution.cpp Sat Jul 12 02:41:32  
> 2008
> @@ -1431,6 +1431,10 @@
>     SCEVHandle HowManyLessThans(SCEV *LHS, SCEV *RHS, const Loop *L,
>                                 bool isSigned);
>
> +    /// executesAtLeastOnce - Test whether entry to the loop is  
> protected by
> +    /// a conditional between LHS and RHS.
> +    bool executesAtLeastOnce(const Loop *L, bool isSigned, SCEV  
> *LHS, SCEV *RHS);
> +
>     /// getConstantEvolutionLoopExitValue - If we know that the  
> specified Phi is
>     /// in the header of its containing loop, we know the loop  
> executes a
>     /// constant number of times, and the PHI node is just a  
> recurrence
> @@ -2612,6 +2616,70 @@
>   return UnknownValue;
> }
>
> +/// executesAtLeastOnce - Test whether entry to the loop is  
> protected by
> +/// a conditional between LHS and RHS.
> +bool ScalarEvolutionsImpl::executesAtLeastOnce(const Loop *L, bool  
> isSigned,
> +                                               SCEV *LHS, SCEV  
> *RHS) {
> +  BasicBlock *Preheader = L->getLoopPreheader();
> +  BasicBlock *PreheaderDest = L->getHeader();
> +  if (Preheader == 0) return false;
> +
> +  BranchInst *LoopEntryPredicate =
> +    dyn_cast<BranchInst>(Preheader->getTerminator());
> +  if (!LoopEntryPredicate) return false;
> +
> +  // This might be a critical edge broken out.  If the loop  
> preheader ends in
> +  // an unconditional branch to the loop, check to see if the  
> preheader has a
> +  // single predecessor, and if so, look for its terminator.
> +  while (LoopEntryPredicate->isUnconditional()) {
> +    PreheaderDest = Preheader;
> +    Preheader = Preheader->getSinglePredecessor();
> +    if (!Preheader) return false;  // Multiple preds.
> +
> +    LoopEntryPredicate =
> +      dyn_cast<BranchInst>(Preheader->getTerminator());
> +    if (!LoopEntryPredicate) return false;
> +  }
> +
> +  ICmpInst *ICI = dyn_cast<ICmpInst>(LoopEntryPredicate- 
> >getCondition());
> +  if (!ICI) return false;
> +
> +  // Now that we found a conditional branch that dominates the  
> loop, check to
> +  // see if it is the comparison we are looking for.
> +  Value *PreCondLHS = ICI->getOperand(0);
> +  Value *PreCondRHS = ICI->getOperand(1);
> +  ICmpInst::Predicate Cond;
> +  if (LoopEntryPredicate->getSuccessor(0) == PreheaderDest)
> +    Cond = ICI->getPredicate();
> +  else
> +    Cond = ICI->getInversePredicate();
> +
> +  switch (Cond) {
> +  case ICmpInst::ICMP_UGT:
> +    if (isSigned) return false;
> +    std::swap(PreCondLHS, PreCondRHS);
> +    Cond = ICmpInst::ICMP_ULT;
> +    break;
> +  case ICmpInst::ICMP_SGT:
> +    if (!isSigned) return false;
> +    std::swap(PreCondLHS, PreCondRHS);
> +    Cond = ICmpInst::ICMP_SLT;
> +    break;
> +  case ICmpInst::ICMP_ULT:
> +    if (isSigned) return false;
> +    break;
> +  case ICmpInst::ICMP_SLT:
> +    if (!isSigned) return false;
> +    break;
> +  default:
> +    return false;
> +  }
> +
> +  if (!PreCondLHS->getType()->isInteger()) return false;

This check looks redundant; a ICMP_[SU][LG]T that's used by a  
conditional
branch will always have integer operands. Can you make this an assert
instead?

>
> +
> +  return LHS == getSCEV(PreCondLHS) && RHS == getSCEV(PreCondRHS);
> +}
> +
> /// HowManyLessThans - Return the number of times a backedge  
> containing the
> /// specified less-than comparison will execute.  If not computable,  
> return
> /// UnknownValue.
> @@ -2640,12 +2708,17 @@
>
>     // Then, we get the value of the LHS in the first iteration in  
> which the
>     // above condition doesn't hold.  This equals to max(m,n).

This comment is now stale. Can you update it to reflect the
new logic?

Thanks,

Dan

>
> -    SCEVHandle End = isSigned ? SE.getSMaxExpr(RHS, Start)
> -                              : SE.getUMaxExpr(RHS, Start);
> -
> -    // Finally, we subtract these two values to get the number of  
> times the
> -    // backedge is executed: max(m,n)-n.
>
> -    return SE.getMinusSCEV(End, Start);
> +    if (executesAtLeastOnce(L, isSigned,
> +                            SE.getMinusSCEV(AddRec->getOperand(0),  
> One), RHS))
> +      return SE.getMinusSCEV(RHS, Start);
> +    else {
> +      SCEVHandle End = isSigned ? SE.getSMaxExpr(RHS, Start)
> +                                : SE.getUMaxExpr(RHS, Start);
> +
> +      // Finally, we subtract these two values to get the number of  
> times the
> +      // backedge is executed: max(m,n)-n.
> +      return SE.getMinusSCEV(End, Start);
> +    }
>   }