[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.

Hi Silviu,

This is looking close to ready to land; I mostly have minor comments except on the `ExitNotTakenExtras` structure, which is more complex than I had had in mind.


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:546
@@ +545,3 @@
+    };
+    /// Forward declaration of ExitNotTakenExtras
+    struct ExitNotTakenExtras;
----------------
Leave a line between declarations.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:566
@@ +565,3 @@
+
+      void setIncomplete() { ExtraInfo.setInt(1); }
+
----------------
This is pretty obvious by name, but I'd add a doxygen comment for consistency.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:571
@@ +570,3 @@
+      SCEVUnionPredicate *getPred() const {
+        if (ExtraInfo.getPointer()) {
+          return &(ExtraInfo.getPointer()->Pred);
----------------
I'd remove the braces.  Also a more LLVM-idiomatic way of writing this would be

```
if (auto *Info = ExtraInfo.getPointer())
  return &Info->Pred;
return nullptr;
```


================
Comment at: include/llvm/Analysis/ScalarEvolution.h:580
@@ +579,3 @@
+      bool hasAlwaysTruePred() const {
+        if (!getPred())
+          return true;
----------------
I'd mildly prefer `return !getPred() || getPred()->isAlwaysTrue();`, but if you like this better, that's fine too.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:586
@@ +585,3 @@
+      /// Defines a simple forward iterator for ExitNotTakenInfo.
+      class ExitNotTakenInfoIterator
+          : public std::iterator<std::forward_iterator_tag, ExitNotTakenInfo> {
----------------
This is great!

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:645
@@ +644,3 @@
+      /// Iterators
+      ExitNotTakenInfoIterator begin() const {
+        return ExitNotTakenInfoIterator(this, 0);
----------------
The following does **not** need to be done for this change, but this is
just to note some cleanup work one of us should consider taking up in
the future:

Now that we're being so nice and well factored about this, I think it
will be even nicer if we have a good containment relationship.
Specifically, it will be nice to have a top level `ExitTakenInfoSet`
struct that contains one inlined `ExitTakenInfo` object, and a
`PointerIntPair` of `ExitTakenInfoSetExtras` and the `isComplete` bit.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:656
@@ +655,3 @@
+      /// The predicate associated with the ExitNotTakenInfo struct.
+      SCEVUnionPredicate Pred;
+      /// The extra exits in the loop. Only the ExitNotTakenExtras structure
----------------
I think the code would be cleaner if we changed `Pred` to a `std::vector` or `SmallVector` of predicates, and have the invariant be:

 - `ExtraInfo` in the "root" `ExitNotTaken` instance is `nullptr` means that there is one unpredicated exit count
 - `ExtraInfo` in the "root" `ExitNotTaken` instance is not `nullptr` means that i'th exit count is in `i == 0 ? Root : Root->Extra.Exits[i - 1]` (exactly as it is today), and the predicate for the i'th exit count is `Root->Extra ? Root->Extra.Exits[i] : AlwaysTrue`.

This is less efficient than the layout you have here, but is simpler; and we still have the nice property that unpredicated exit counts for single exit loops can be represented compactly.

Does the above sound feasible?  If it does, then lets please go ahead with that; else let me know and I'll review the `BackedgeTakenInfo` constructor as is.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:657
@@ +656,3 @@
+      SCEVUnionPredicate Pred;
+      /// The extra exits in the loop. Only the ExitNotTakenExtras structure
+      /// pointed to by the first ExitNotTakenInfo struct (associated with the
----------------
Again, please leave blank lines between decls.

================
Comment at: include/llvm/Analysis/ScalarEvolution.h:870
@@ -734,1 +869,3 @@
+                                       bool IsSubExpr,
+                                       bool AllowPredicates);
 
----------------
Please consistently use one of `AllowPredicates` or `CreatePredicates`.  Also there is some inconsistency around when `CreatePredicates` has a default value and when it does not -- is there a reason for the difference?

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5306
@@ +5305,3 @@
+
+  RemoveLoopFromBackedgeMap(BackedgeTakenCounts);
+  RemoveLoopFromBackedgeMap(PredicatedBackedgeTakenCounts);
----------------
sbaranga wrote:
> Sanjoy, I've left this as is for now. Let me know if you prefer the form above, and I'll try to change the code to use it.
sgtm

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5453
@@ -5420,4 +5452,3 @@
 
-  ExitNotTaken.ExitingBlock = ExitCounts[0].first;
-  ExitNotTaken.ExactNotTaken = ExitCounts[0].second;
-  if (NumExits == 1) return;
+  ExitNotTaken.ExitingBlock = std::get<0>(ExitCounts[0]);
+  ExitNotTaken.ExactNotTaken = std::get<1>(ExitCounts[0]);
----------------
Can you please do one of the following:

 - Comment which numeric field is which
 - Cache the three fields in three local variables, with more mnemonic names

Honestly, reading the code, a `struct` with named fields would have been better; but we can fix that later.

================
Comment at: lib/Analysis/ScalarEvolution.cpp:5459
@@ +5458,3 @@
+  if (NumExits > 1)
+    PredsSize = 1 + std::count_if(std::next(ExitCounts.begin()),
+                                  ExitCounts.end(), [](EdgeInfo &Entry) {
----------------
Are you unconditionally assuming `!std::get<2>(ExitCounts[0]).isAlwaysTrue()` here (given that you're unconditionally adding `1`)?  If so, please add a brief comment on why that is okay.


http://reviews.llvm.org/D17201