[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.
This revision now requires changes to proceed.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:557
@@ -545,1 +556,3 @@
+      /// otherwise it's all CouldNotCompute.
+      SCEVUnionPredicate Pred;
 
----------------
Now that you're embedding `SCEVUnionPredicate` in every
`ExitNotTakenInfo`, I'm somewhat worried about the `SmallVector<const
SCEVPredicate *, 16> Preds;` :)

Firstly, what is the typical number of predicates we see?  16 seems
higher that what I would expect.

Secondly, a better (in terms of memory consumption) solution would be
to have a `std::unique_ptr<SCEVUnionPredicate>` here, where a null
value means "always true" so that in the non-predicated case we only
use up one word.

The most compact solution (I think we should go with this) is to:

 - Extract out a `struct ExitNotTakenExtras` that contains a
   `SmallVector` of `ExitNotTaken` s and a `SCEVUnionPredicate`

 - Change `PointerIntPair<ExitNotTakenInfo*, 1> NextExit` to
   `PointerIntPair<ExitNotTakenExtras *, 1> NextExit` where it is
   usually null, but if it is non-null you either have multiple exits
   or have some SCEV predicates.

Normally I wouldn't worry so much about space, but `ExitNotTakenInfo`
has clearly been optimized for space usage, so we should try not to
break that.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:586
@@ -572,3 +585,3 @@
       /// loop that is known, or a SCEVCouldNotCompute.
       const SCEV *Max;
 
----------------
Add a comment here that `Max` is valid only if the predicates on each of the `ExitNotTakenInfo` instances is true.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:593
@@ -579,3 +592,3 @@
       BackedgeTakenInfo(
-        SmallVectorImpl< std::pair<BasicBlock *, const SCEV *> > &ExitCounts,
-        bool Complete, const SCEV *MaxCount);
+        SmallVectorImpl<std::pair<BasicBlock *, const SCEV *>> &ExitCounts,
+        SmallVectorImpl<SCEVUnionPredicate *> &ExitPreds, bool Complete,
----------------
I think the right API here is to promote the `std::pair` into a full struct that carries the block, backedge count and a predicate; and not have a separate vector for `ExitPreds`.  You could also use `std::tuple` here (since the types are all different, there isn't much scope for confusion).

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:610
@@ +609,3 @@
+      /// loop if it is known and always correct (independent of any
+      /// assumptions that should be checked at run-time), or
+      /// SCEVCouldNotCompute otherwise. This is the number of times the loop
----------------
Can you please make this comment a little more clear?  Is it that the caller needs to know in advance, by some other means, that the backedge taken count is predicated and pass in a non-null `Predicates`?  If so, what is that other means?

The `always correct (independent of any assumptions that should be checked at run-time)`  is misleading too -- you do sometimes return exact BE counts are **dependent** on a run-time assumption.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:639
@@ -614,1 +638,3 @@
     DenseMap<const Loop*, BackedgeTakenInfo> BackedgeTakenCounts;
+    /// Cache the predicated backedge-taken count of the loops for this
+    /// function as they are computed.
----------------
Here and elsewhere, can you please add a newline after field declarations (i.e. after the declaration for `BackedgeTakenCounts`)?

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:750
@@ +749,3 @@
+    /// taken count will be known).
+    const BackedgeTakenInfo &getPredicatedBackedgeTakenInfo(const Loop *L);
+
----------------
I'd remove the `(the exact backedge taken count will be known)` bit -- it can otherwise mislead people into thinking that this will return a precise be count, no matter what.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:766
@@ -727,2 +765,3 @@
     /// execute if its exit condition were a conditional branch of ExitCond,
-    /// TBB, and FBB.
+    /// TBB, and FBB. If IsGuarded is set, this call try to use a minimal set
+    /// of SCEV predicates in order to return an exact answer.
----------------
Nit: the parameter name is `IsGuarded` and not `Guarded`.  But, as I said earlier, it is probably better to rename this to `AddPredicates`, `AllowPredicates` or `CreatePredicates` as you have below.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:1227
@@ +1226,3 @@
+    const SCEV *getPredicatedBackedgeTakenCount(const Loop *L,
+                                             SCEVUnionPredicate &Predicates);
+
----------------
Nit: indentation.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5265
@@ +5264,3 @@
+  auto RemoveLoopFromBackedgeMap =
+    [L] (DenseMap<const Loop*, BackedgeTakenInfo> &Map) {
+    auto BTCPos = Map.find(L);
----------------
Usually I see lambdas formatted with no space between the `]` and the `(`.  I'd say just run the diff through `clang-format` before checkin, and whatever it does is fine. :)

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5273
@@ +5272,3 @@
+
+  RemoveLoopFromBackedgeMap(BackedgeTakenCounts);
+  RemoveLoopFromBackedgeMap(PredicatedBackedgeTakenCounts);
----------------
What you have here is fine, but I'd have tried:


```
for (auto &Map : {BackedgeTakenCounts, PredicatedBackedgeTakenCounts}) {
  // Remove L from Map
}
```

(not 100% sure if the above will work).

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5478
@@ -5426,3 +5477,3 @@
     BasicBlock *ExitBB = ExitingBlocks[i];
-    ExitLimit EL = computeExitLimit(L, ExitBB);
+    ExitLimit EL = computeExitLimit(L, ExitBB, Guarded);
 
----------------
Add an assert here that if `Guarded` is false, then the predicate in `EL` is trivially true (i.e. we didn't add a predicate when we were not supposed to).

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5488
@@ -5436,1 +5487,3 @@
       ExitCounts.push_back({ExitBB, EL.Exact});
+      ExitCountPreds.push_back(&EL.Pred);
+    }
----------------
This does not look right to me -- won't the `EL` be freed after each iteration, leaving a dangling pointer in `ExitCountPreds`?


http://reviews.llvm.org/D17201