[PATCH] D93317: [LV] Vectorize (some) early and multiple exit loops

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Nice leverage of requiresScalarEpilogue!
Looks good to me, adding some minor comments.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1098
 
-  // We must have a single exiting block.
-  if (!Lp->getExitingBlock()) {
-    reportVectorizationFailure("The loop must have an exiting block",
+  // We must have a single exiting block.  Note that this allows multiple
+  // exits provided they all exit to the same block.
----------------
To avoid confusing exit/ing block terms:
`// We currently must have a single "exit block" after the loop. Note that multiple "exiting blocks" inside the loop are allowed, provided they all reach the single exit block.`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1105
+  if (!ExitBB) {
+    reportVectorizationFailure("The loop must have an unique exit block",
         "loop control flow is not understood by vectorizer",
----------------
nit: a[n] unique


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:1120
+  if (!empty(ExitBB->phis()) && !ExitBB->getSinglePredecessor()) {
+    reportVectorizationFailure("The loop must have an unique exit block",
         "loop control flow is not understood by vectorizer",
----------------
Clarify failure reason, e.g., "The loop must have no live-out values if it has more than one exiting block" ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1558
+    // iteration in scalar form.
+    if (!TheLoop->getExitingBlock() || !TheLoop->isRotatedForm())
+      return true;
----------------
Checking that the only exit is the latch can be done (alternatively, literally) by
`if (TheLoop->getExitingBlock() == TheLoop->getLoopLatch())`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3014
+  // 2) If any instruction may follow a conditionally taken exit. (e.g. due to
+  //    a multi exit loop, or a non-bottom tested single exit loop)
   if (VF.isVector() && Cost->requiresScalarEpilogue()) {
----------------
To avoid confusing exit/ing block/loop terms:
`That is, if the loop contains multiple exiting blocks, or a single exiting block which is not the latch.`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5504-5505
 
-  // Now try the tail folding
+  // We can't vectorize anything but a bottom tested loop without a scalar
+  // epilogue.  Unless this is bottom tested, bail out.  We'd have to handle
+  // the fact that not every instruction executes on the last ieration.  This
----------------
>   // We can't vectorize anything but a bottom tested loop without a scalar epilogue.
Perhaps better stated as:
`  // The only loops we can vectorize without a scalar epilogue, are loops with a bottom-test and a single exiting block.`


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5506
+  // epilogue.  Unless this is bottom tested, bail out.  We'd have to handle
+  // the fact that not every instruction executes on the last ieration.  This
+  // will require a lane mask which varies through the vector loop body.  (TODO)
----------------
nit: i[t]eration


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5509
+  if (!TheLoop->getExitingBlock() || !TheLoop->isRotatedForm())
+    return None;
 
----------------
If predication is preferred over a scalar epilog, but the latter is not forbidden (i.e., the CM_ScalarEpilogueNotNeededUsePredicate case), we could "fallback to a vectorization with a scalar epilogue" here, instead of bailing out, as done below?

Can test `if (TheLoop->getExitingBlock() != TheLoop->getLatchBlock())`


================
Comment at: llvm/test/Transforms/LoopVectorize/loop-legality-checks.ll:24
-}
-
 ; Make sure LV legal bails out when there is no exiting block
----------------
Would it be useful to keep this (single exiting, double latched(?)) test?


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

https://reviews.llvm.org/D93317