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

[Tim Shen via llvm-commits]

timshen added inline comments.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:249-254
@@ +248,8 @@
+
+  struct LoopBodyFilter {
+    bool operator()(NodeRef N) const {
+      const Loop *L = N.first;
+      return N.second != L->getHeader() && L->contains(N.second);
+    }
+  };
+
----------------
anemet wrote:
> Can this be a predicate function instead?
If the predicate is a function, then to use it we have to pass it around as a function pointer, which is more expensive because of the dynamic dispatch. If we have a huge loop body it may end up with effecting the performance.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:262-265
@@ +261,6 @@
+  static ChildIteratorType child_begin(NodeRef Node) {
+    return make_filter_range(make_range<WrappedSuccIterator>(
+                                 {succ_begin(Node.second), Node.first},
+                                 {succ_end(Node.second), Node.first}),
+                             LoopBodyFilter{})
+        .begin();
----------------
anemet wrote:
> Actually a more basic question.  Why can't we filter succ_iterator directly i.e. just by passing the loop to LoopBodyFilter here?
Because we want to do the filtering lazily, don't we? If we filter succ_iterator here by LoopBodyFilter, we have to create a container to hold the filtered results (so that later operator++() and operator*() can inspect those results), which is less efficient. I went for the efficient way without sacrifice the readability (?).

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1919-1924
@@ -1845,5 +1918,8 @@
 
-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;
+  }
 
----------------
anemet wrote:
> This would be the place to emit the optimization remark (emitAnalysis)
There are two problems revealed by this addInnerLoop -> addAcyclicInnerLoop change.

First, since addAcyclicInnerLoop gets called in LoopVectorizePass::runImpl, where no report/analysis infrastructure is set up, it's pretty hard to call emitAnalysis here.

Secondly, a loop with a cycle in its body never gets into LoopVectorizePass::processLoop, so nothing is logged through -pass-remarks-missed. That's why I have to check for -pass-remarks in the test case.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:4635-4638
@@ -4558,1 +4634,6 @@
 
+  if (hasCyclesInLoopBody(*TheLoop)) {
+    emitAnalysis(VectorizationReport() << "the loop body has a cycle");
+    return false;
+  }
+
----------------
anemet wrote:
> You no longer need this.
Could LoopVectorizationLegality be used in anywhere else, where addAcyclicInnerLoop doesn't guard?

================
Comment at: test/Transforms/LoopVectorize/pr28541.ll:1
@@ +1,2 @@
+; RUN: opt -loop-vectorize -pass-remarks=loop-vectorize -S < %s 2>&1 | FileCheck %s
+
----------------
anemet wrote:
> Why did you change from -pass-remaks-missed to -pass-remarks?  I prefer positive checks over negative ones.
> 
> I think that we also want to check the particular reason for failing to vectorize, so please also pass -pass-remarks-analysis=loop-vectorize and check for that string as well.
> Why did you change from -pass-remaks-missed to -pass-remarks? I prefer positive checks over negative ones.

See the comments for addAcyclicInnerLoop.


https://reviews.llvm.org/D22952