[PATCH] D17201: [SCEV] Introduce a guarded backedge taken count and use it in LAA and LV

[Sanjoy Das via llvm-commits]

sanjoy added a comment.

Hi Silviu,

In http://reviews.llvm.org/D17201#371737, @sbaranga wrote:

> Was this what you had in mind, or do you think we should also be doing something else?


Yes, this is what I had in mind, so thanks!  I'm still not done
reviewing the change, but I've added some minor comments inline.  I'll
try to finish the review by Monday (no need to address the inline
comments before then).

Btw, it would be great to have (in a later change) similar `llvm-lit`
tests for `PredicatedScalarEvolution::getAsAddRec` as well; but I'll
leave it to you to make a call on that.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:753
@@ -719,2 +752,3 @@
     /// Compute the number of times the specified loop will iterate.
-    BackedgeTakenInfo computeBackedgeTakenCount(const Loop *L);
+    /// If IsGuarded is set, we will create new SCEV predicates as necessary in
+    /// order to return an exact answer.
----------------
This is minor, but the `IsGuarded` name is somewhat misleading, since the loop may not already be guarded.  Why not call this `AddPredicates` or `AllowPredicates`?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5173
@@ +5172,3 @@
+
+  std::pair<DenseMap<const Loop *, BackedgeTakenInfo>::iterator, bool> Pair =
+      PredicatedBackedgeTakenCounts.insert({L, BackedgeTakenInfo()});
----------------
Use `auto` here.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5179
@@ +5178,3 @@
+
+  BackedgeTakenInfo Result = computeBackedgeTakenCount(L, true);
+
----------------
Add a `/* IsGuarded = */` before the `true`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5719
@@ +5718,3 @@
+    return computeExitLimitFromICmp(L, ExitCondICmp, TBB,
+                                    FBB, ControlsExit, true);
+  }
----------------
Please add a comment as `/* IsGuarded = */ true`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:6879
@@ +6878,3 @@
+    // Try to make this a chrec using runtime tests.
+    const SCEV *Expr = convertSCEVToAddRecWithPredicates(V, L, P);
+    AddRec = dyn_cast<SCEVAddRecExpr>(Expr);
----------------
No need to fix this in this change, but a nicer API could be to have `convertSCEVToAddRecWithPredicates` return an add recurrence or null if it failed (and get rid of the `dyn_cast`).

================
Comment at: lib/Analysis/ScalarEvolution.cpp:8495
@@ +8494,3 @@
+    IV = dyn_cast<SCEVAddRecExpr>(Expr);
+    if (!IV)
+      return getCouldNotCompute();
----------------
I think you can just fall through the logic below (under `// Avoid weird loops`).  Also, the comment on the api change to `convertSCEVToAddRecWithPredicates` also applies here.


http://reviews.llvm.org/D17201