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

[silviu.baranga@arm.com via llvm-commits]

sbaranga added inline comments.

================
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
----------------
sanjoy wrote:
> 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.
The problem with this is we can't get the predicate of an ExitNotTaken without knowing the root, so the information there is not self contained anymore.

So things like the isPredicateAlwaysTrue() method would need to change (because we don't know the root), probably by taking the ExitNotTaken root as the argument?

Would there be a way around this? If not, I would prefer to keep the current solution.

================
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) {
----------------
sanjoy wrote:
> 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.
No, we know the first ExitNotTakenInfo has an ExtraInfo struct because we have more than 1 exit (so there's no need to check the predicate). The other entries only have an ExtraInfo if they have a not always true SCEVPredicate.

Perhaps ExtraInfoSize instead of PredsSize would have been better here.


http://reviews.llvm.org/D17201