[PATCH] D92066: [LAA] Relax restrictions on early exits in loop structure

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

This looks good to me, adding some minor comments. Thanks!

> I can't actually find anything in LAA itself which relies on having all instructions within a loop execute an equal number of times.

Well, LAA's `RuntimePointerChecking::insert()` relies on `getBackedgeTakenCount()` to provide "the" trip-count, which may vary for distinct instructions if the loop is not bottom-tested. But it does provide an upper-bound for all instructions, i.e., a safe, conservative value. In other words, a slightly tighter runtime check could potentially be provided for pointers accessed one less iteration, appearing 'below' the exiting block.

> (where duplicates don't already exist)

LV indeed checks for single-exit-at-bottom by itself, in LVL.canVectorize*(). Curious to see this restriction being lifted! It is somewhat related to "massaging" inner loops when vectorizing an outer loop.



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:1789
-    recordAnalysis("CFGNotUnderstood")
-        << "loop control flow is not understood by analyzer";
-    return false;
----------------
Note: these DEBUG messages get lost.


================
Comment at: llvm/lib/Transforms/Scalar/LoopDistribute.cpp:673
 
     if (!L->getExitBlock())
       return fail("MultipleExitBlocks", "multiple exit blocks");
----------------
This existing !getExitBlock() check indeed saves us from introducing an additional !getExitingBlock() check; worth a comment, here and/or in LoopInfo?
Note that L may have a single exit block B and multiple exiting blocks - all jumping to B. But getExitBlock() returns false for such an L, in contrast to getUniqueExitBlock() which returns true. I.e., getExitBlock() does imply getExitingBlock().


================
Comment at: llvm/lib/Transforms/Scalar/LoopDistribute.cpp:679
+    if (!L->isRotatedForm())
+      return fail("NotBottomTested",
+                  "loop is not bottom tested");
----------------
May be worth noting in the patch summary that some ORE and LAA debug messages are added and dropped, respectively.


================
Comment at: llvm/lib/Transforms/Scalar/LoopDistribute.cpp:684
 
     // LAA will check that we only have a single exiting block.
     LAI = &GetLAA(*L);
----------------
Hoist or remove above comment?


================
Comment at: llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp:630
   for (Loop *L : Worklist) {
+    // Match historical behavior
+    if (!L->isRotatedForm() || !L->getExitingBlock())
----------------
Does this (TODO?) comment imply that in present/future this limitation can/should be dropped?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92066/new/

https://reviews.llvm.org/D92066