[SCEV] Adding a check on outgoing branches of a terminator instr for SCEVBackedgeConditionFolder, NFC.

[jbhateja]

Summary

For a given loop, getLoopLatch returns a non-null value
when a loop has only one latch block. In the modified
context adding an assertion to check that both the outgoing branches of
a terminator instruction (of latch) does not target same header.
+
few minor code reorganization.

[sanjoy]

llvm/trunk/lib/Analysis/ScalarEvolution.cpp

4095–4097

Can you please add an assert here that BI->getSuccessor(0) != BI->getSuccessor(1)? That way we'll know to update this place if/when getLoopLatch() becomes smarter.

[jbhateja]

llvm/trunk/lib/Analysis/ScalarEvolution.cpp

4095–4097

I guess we will have to look at all the places from where a call to getLoopLatch is made, adding such an assert at one place does not looks reasonable.

[sanjoy]

llvm/trunk/lib/Analysis/ScalarEvolution.cpp

4095–4097

adding such an assert at one place does not looks reasonable

I'm not sure I agree. Here we rely on BI->getSuccessor(0) != BI->getSuccessor(1) implied from getLoopLatch() for correctness, and asserting that precondition seems like an incremental improvement, irrespective of what we do for the other places.

Secondly, I don't think asserting BI->getSuccessor(0) != BI->getSuccessor(1) everywhere we call getLoopLatch() is a good idea anyway. We should assert BI->getSuccessor(0) != BI->getSuccessor(1) where it matters to the code that called getLoopLatch(), not to all places that call getLoopLatch().

[jbhateja]

Few more minor changes , mostly NFCs.

llvm/trunk/lib/Analysis/ScalarEvolution.cpp

while (!Worklist.empty()) {
     PushDefUseChildren(I, Worklist);
   }
 }
 
 namespace {
 
 class SCEVInitRewriter : public SCEVRewriteVisitor<SCEVInitRewriter> {
 public:
-  SCEVInitRewriter(const Loop *L, ScalarEvolution &SE)
-      : SCEVRewriteVisitor(SE), L(L) {}
-
   static const SCEV *rewrite(const SCEV *S, const Loop *L,
                              ScalarEvolution &SE) {
     SCEVInitRewriter Rewriter(L, SE);
     const SCEV *Result = Rewriter.visit(S);
     return Rewriter.isValid() ? Result : SE.getCouldNotCompute();
   }
 
   const SCEV *visitUnknown(const SCEVUnknown *Expr) {
     if (!SE.isLoopInvariant(Expr, L))
       Valid = false;
     return Expr;
   }
 
   const SCEV *visitAddRecExpr(const SCEVAddRecExpr *Expr) {
     // Only allow AddRecExprs for this loop.
     if (Expr->getLoop() == L)
       return Expr->getStart();
     Valid = false;
     return Expr;
   }
 
   bool isValid() { return Valid; }
 
 private:
+  explicit SCEVInitRewriter(const Loop *L, ScalarEvolution &SE)
+      : SCEVRewriteVisitor(SE), L(L) {}
+
   const Loop *L;
   bool Valid = true;
 };
 
 /// This class evaluates the compare condition by matching it against the
 /// condition of loop latch. If there is a match we assume a true value
 /// for the condition while building SCEV nodes.
 class SCEVBackedgeConditionFolder
     : public SCEVRewriteVisitor<SCEVBackedgeConditionFolder> {
 public:
   static const SCEV *rewrite(const SCEV *S, const Loop *L,
                              ScalarEvolution &SE) {
     bool IsPosBECond = false;
     Value *BECond = nullptr;
     if (BasicBlock *Latch = L->getLoopLatch()) {
       BranchInst *BI = dyn_cast<BranchInst>(Latch->getTerminator());
       if (BI && BI->isConditional()) {
+        assert(BI->getSuccessor(0) != BI->getSuccessor(1) &&
+               "Both outgoing branches should not target same header!");
         BECond = BI->getCondition();
         IsPosBECond = BI->getSuccessor(0) == L->getHeader();
       } else {
         return S;
       }
     }
     SCEVBackedgeConditionFolder Rewriter(L, BECond, IsPosBECond, SE);
     return Rewriter.visit(S);
SCEVBackedgeConditionFolder::compareWithBackedgeCondition(Value *IC) {
   if (BackedgeCond == IC)
     return IsPositiveBECond ? SE.getOne(Type::getInt1Ty(SE.getContext()))
                             : SE.getZero(Type::getInt1Ty(SE.getContext()));
   return None;
 }
 
 class SCEVShiftRewriter : public SCEVRewriteVisitor<SCEVShiftRewriter> {
 public:
-  SCEVShiftRewriter(const Loop *L, ScalarEvolution &SE)
-      : SCEVRewriteVisitor(SE), L(L) {}
-
   static const SCEV *rewrite(const SCEV *S, const Loop *L,
                              ScalarEvolution &SE) {
     SCEVShiftRewriter Rewriter(L, SE);
     const SCEV *Result = Rewriter.visit(S);
     return Rewriter.isValid() ? Result : SE.getCouldNotCompute();
   }
 
   const SCEV *visitUnknown(const SCEVUnknown *Expr) {
     // Only allow AddRecExprs for this loop.
     if (!SE.isLoopInvariant(Expr, L))
       Valid = false;
     return Expr;
   }
 
   const SCEV *visitAddRecExpr(const SCEVAddRecExpr *Expr) {
     if (Expr->getLoop() == L && Expr->isAffine())
       return SE.getMinusSCEV(Expr, Expr->getStepRecurrence(SE));
     Valid = false;
     return Expr;
   }
 
   bool isValid() { return Valid; }
 
 private:
+  explicit SCEVShiftRewriter(const Loop *L, ScalarEvolution &SE)
+      : SCEVRewriteVisitor(SE), L(L) {}
+
   const Loop *L;
   bool Valid = true;
 };
 
 } // end anonymous namespace
 
 SCEV::NoWrapFlags
 ScalarEvolution::proveNoWrapViaConstantRanges(const SCEVAddRecExpr *AR) {
const SCEVPredicate *ScalarEvolution::getWrapPredicate(
   UniquePreds.InsertNode(OF, IP);
   return OF;
 }
 
 namespace {
 
 class SCEVPredicateRewriter : public SCEVRewriteVisitor<SCEVPredicateRewriter> {
 public:
-  SCEVPredicateRewriter(const Loop *L, ScalarEvolution &SE,
-                        SmallPtrSetImpl<const SCEVPredicate *> *NewPreds,
-                        SCEVUnionPredicate *Pred)
-      : SCEVRewriteVisitor(SE), NewPreds(NewPreds), Pred(Pred), L(L) {}
 
   /// Rewrites \p S in the context of a loop L and the SCEV predication
   /// infrastructure.
   ///
   /// If \p Pred is non-null, the SCEV expression is rewritten to respect the
   /// equivalences present in \p Pred.
   ///
   /// If \p NewPreds is non-null, rewrite is free to add further predicates to
if (AR && AR->getLoop() == L && AR->isAffine()) {
         return SE.getAddRecExpr(SE.getSignExtendExpr(AR->getStart(), Ty),
                                 SE.getSignExtendExpr(Step, Ty), L,
                                 AR->getNoWrapFlags());
     }
     return SE.getSignExtendExpr(Operand, Expr->getType());
   }
 
 private:
+  explicit SCEVPredicateRewriter(const Loop *L, ScalarEvolution &SE,
+                        SmallPtrSetImpl<const SCEVPredicate *> *NewPreds,
+                        SCEVUnionPredicate *Pred)
+      : SCEVRewriteVisitor(SE), NewPreds(NewPreds), Pred(Pred), L(L) {}
+
   bool addOverflowAssumption(const SCEVPredicate *P) {
     if (!NewPreds) {
       // Check if we've already made this assumption.
       return Pred && Pred->implies(P);
     }
     NewPreds->insert(P);
     return true;
   }