[PATCH] D15559: [LoopUnrollRuntime] Do unroll when IV's setp is one or minus one

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1828
@@ +1827,3 @@
+static bool IsExistExpr(const SCEV *S, const SCEV *Base) {
+  if (S == Base)
+    return true;
----------------
A better name for this function would be `IsContainedIn`.  And this should use `SCEVTraversal`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1854
@@ -1837,8 +1853,3 @@
 
-    if (!match(BB->getTerminator(),
-               m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Instruction(RHS)),
-                    TrueBB, FalseBB)))
-      continue;
-
-    if (SE.getSCEV(LHS) == S && SE.DT.dominates(LHS, At))
-      return LHS;
+    bool IsMatchCmpII =
+        match(BB->getTerminator(),
----------------
I don't see why you need to `match` twice?  You should just be able to do

```
match(BB->getTerminator(), m_Br(m_ICmp(Pred, m_Instruction(LHS), m_Value(RHS)))
```

with `Value *RHS`.  Then you can make your later code conditional on `isa<Instruction>(RHS)`.

================
Comment at: lib/Analysis/ScalarEvolutionExpander.cpp:1884
@@ +1883,3 @@
+                  SCConst->getValue()->isMinusOne())
+                return LHS;
+            }
----------------
I don't understand this part -- how do you conclude that `LHS` is a correct expansion for `S`?  E.g. what if `getSCEV(LHS)` is `{S,+,1}`?  Then clearly the expansion of `S` isn't `LHS`.

I think you need to do a (recursive) search of the IR level operands of `LHS` and `RHS` to see if any of them are congruent to `S`.

================
Comment at: lib/Transforms/Utils/LoopUnrollRuntime.cpp:316
@@ -315,2 +315,3 @@
   SCEVExpander Expander(*SE, DL, "loop-unroll");
-  if (!AllowExpensiveTripCount && Expander.isHighCostExpansion(TripCountSC, L))
+  BranchInst *ExtingBlockBR =
+      cast<BranchInst>(L->getExitingBlock()->getTerminator());
----------------
I don't think this is the right value for `At`.  `At` should be the location you want to expand the SCEV expression at, which in this case is `PreHeaderBR`.


http://reviews.llvm.org/D15559