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

[Adam Nemet via llvm-commits]

anemet added inline comments.

================
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)
----------------
timshen wrote:
> 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.
OK.  There is no regression in the optimization diagnostic quality with this patch so I am fine with doing it later.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4638-4641
@@ -4558,1 +4637,6 @@
 
+  if (hasCyclesInLoopBody(*TheLoop)) {
+    emitAnalysis(VectorizationReport() << "the loop body has a cycle");
+    return false;
+  }
+
----------------
No.

================
Comment at: test/Transforms/LoopVectorize/pr28541.ll:2
@@ +1,3 @@
+; RUN: opt -loop-vectorize -pass-remarks=loop-vectorize -S < %s 2>&1 | FileCheck %s
+
+; Check that opt does not crash on such input:
----------------
Then please add a FIXME about this.


https://reviews.llvm.org/D22952