[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:224-226
@@ -222,1 +223,5 @@
 
+// A traits type that is intended to be used in graph algorithms. The graph it
+// models starts at the loop header, and traverses the BasicBlocks that are in
+// the loop body, but not the loop header.
+struct LoopBodyTraits {
----------------
I think that we want to say it here a bit more explicitly that the reason you skip the loop header so that the backedges are excluded from the graph.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:230
@@ +229,3 @@
+
+  class WrappedSuccIterator
+      : public iterator_adaptor_base<
----------------
Add a comment that this is wrapped in order to 'package' a constant Loop inside the iterator.

================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:246
@@ +245,3 @@
+
+    NodeRef operator*() const { return {this->L, *this->I}; }
+  };
----------------
I don't think we use 'this' in such cases.

================
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);
+    }
+  };
+
----------------
Can this be a predicate function instead?

================
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();
----------------
Actually a more basic question.  Why can't we filter succ_iterator directly i.e. just by passing the loop to LoopBodyFilter here?

================
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;
+  }
 
----------------
This would be the place to emit the optimization remark (emitAnalysis)

================
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;
+  }
+
----------------
You no longer need this.

================
Comment at: test/Transforms/LoopVectorize/pr28541.ll:1
@@ +1,2 @@
+; RUN: opt -loop-vectorize -pass-remarks=loop-vectorize -S < %s 2>&1 | FileCheck %s
+
----------------
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.


https://reviews.llvm.org/D22952