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

[Sanjoy Das via llvm-commits]

sanjoy requested changes to this revision.
sanjoy added a comment.
This revision now requires changes to proceed.

Given that we're now computing predicated trip counts (the bread and butter of SCEV), I think this is good time to start doing some whitebox testing.  What I mean by that is to add a way to get SCEV to produce a predicated trip count and dump both the predicate and the trip count, and do llvm-lit/FileCheck type testing on the output.  Unfortunately, this will be more work for you, but I think this the right thing to do for the system.  We don't want to end up in a situation where adding a test for a predicated trip count related bug involves tricking the loop vectorizer to vectorize a certain loop.

In fact, I think there is already some scope for whitebox testing things like `PredicatedScalarEvolution::getAsAddRec`, maybe that's a good place to start?

Again, I know this is more work for you; but having the infrastructure to easily add tests is important.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:597
@@ -589,4 +596,3 @@
       /// Return an expression indicating the exact backedge-taken count of the
-      /// loop if it is known, or SCEVCouldNotCompute otherwise. This is the
-      /// number of times the loop header can be guaranteed to execute, minus
-      /// one.
+      /// count of the loop if it is known and always correct (independent
+      /// of any assumptions that should be checked at run-time), or
----------------
"count of the" is repeated

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:742
@@ -724,3 +741,3 @@
     /// execute if its exit condition were a conditional branch of the ICmpInst
     /// ExitCond, TBB, and FBB.
     ExitLimit computeExitLimitFromICmp(const Loop *L,
----------------
Describe `UseAssumptions` here.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:787
@@ -769,1 +786,3 @@
+    ExitLimit HowFarToZero(const SCEV *V, const Loop *L, bool IsSubExpr,
+                           bool Force = false);
 
----------------
I don't think `Force` is a good name -- how about `CreateAssumptions` or `CreatePredicates`?  Same comment below.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1178
@@ -1155,1 +1177,3 @@
 
+    const SCEV *getGuardedBackedgeTakenCount(const Loop *L,
+                                             SCEVUnionPredicate &Predicates);
----------------
Add a comment for this (fine to extend the one on `getBackedgeTakenCount`)

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5223
@@ +5222,3 @@
+    }
+    Preds.add(&(ENT->Pred));
+  }
----------------
I don't think you need the parens around `ENT->Pred`.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5683
@@ -5607,1 +5682,3 @@
+  // Try to compute it using assumptions.
+  return computeExitLimitFromICmp(L, ExitCond, TBB, FBB, ControlsExit, true);
 }
----------------
I don't think this is the right layering -- for instance, this forces SCEV clients that don't want speculative / predicated trip counts to pay to cost of computing them.

I'd say SCEV users that care about predicated trip counts should do this retry themselves, i.e. something like

```
const SCEV *TC = getBackedgeTakenCount(L);
if (TC is SCEVCouldNotCompute) {
  SE->forgetLoop(L);
  TC = getPredicatedBackedgeTakenCount(L);
}
```


================
Comment at: lib/Analysis/ScalarEvolution.cpp:6728
@@ -6647,1 +6727,3 @@
   const SCEVAddRecExpr *AddRec = dyn_cast<SCEVAddRecExpr>(V);
+  if ((!AddRec) && Force) {
+    // Try to make this a chrec using runtime tests.
----------------
I don't think you need to `(!A) && B`, you can `!A && B` instead.


http://reviews.llvm.org/D17201