[PATCH] D22952: [LoopVectorize] Detect loops in the innermost loop before creating InnerLoopVectorizer

[Tim Shen via llvm-commits]

timshen marked an inline comment as done.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1922-1928
@@ -1845,6 +1921,9 @@
 
-static void addInnerLoop(Loop &L, SmallVectorImpl<Loop *> &V) {
-  if (L.empty())
-    return V.push_back(&L);
+static void addAcyclicInnerLoop(Loop &L, SmallVectorImpl<Loop *> &V) {
+  if (L.empty()) {
+    if (!hasCyclesInLoopBody(L))
+      V.push_back(&L);
+    return;
+  }
 
   for (Loop *InnerL : L)
----------------
anemet wrote:
> It seem to me that nothing prevents us from letting all loops go all the way until Legality.canVectorize().  Then we can check this property as the very first thing in there.
> 
> If that does not work we can check this earlier in processLoop.
> 
> The part we're missing to report diagnostics in addAcyclicInnerLoop is the Hints which provides the pass name.  Alternatively, we can also construct Hints in addAcyclicInnerLoop.  This is probably the least preferred alternative though because then you need to propagate this back to processLoop.
> It seem to me that nothing prevents us from letting all loops go all the way until Legality.canVectorize(). Then we can check this property as the very first thing in there.

I think the point of addInnerLoop, rather than addAllLoops, is to make the Worklist smaller. So if we want to let all loops go all the way until Legality::canVectorize(), there is a potential compilation performance change and also an analysis change.

I'm fine with changing addInnerLoop to addInnerAcyclicLoop, because it more or less is related to this patch, and it doesn't have a performance impact; but changing the "take innermost loop only" assumption of LoopVectorizePass::processLoop is too much.

I think your concern should be addressed in a separate patch.


https://reviews.llvm.org/D22952