[PATCH] D50820: [VPlan] Implement initial vector code generation support for simple outer loops.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added reviewers: mkuper, hfinkel, Ayal.
fhahn added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2800
 void InnerLoopVectorizer::emitMemRuntimeChecks(Loop *L, BasicBlock *Bypass) {
+  // VPlan-native path does not do any analysis for runtime checks currently.
+  if (EnableVPlanNativePath)
----------------
Should we ever hit this code path in the vplan native path at the moment? If not, I would turn it into an assert.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3857
+    SmallVector<BasicBlock *, 2> ScalarBBPredecessors;
+    for (BasicBlock *BB : predecessors(OrigPhi->getParent()))
+      ScalarBBPredecessors.push_back(BB);
----------------
nit: ScalarBBPredecessors be initialized in the constructor so we don't need this loop (Same for VectorBBPredecessors)? There's a constructor that takes an iterator range.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7186
+  Plan->addVF(VF);
+  for (VF *= 2; VF < Range.End; VF *= 2) {
+    Plan->addVF(VF);
----------------
Could we use `unsigned VF = Range.Start;` at the start of the for loop, so we do not need plan->addVF outside? 


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7400
+  // outer loops - a check for empty loop is sufficient here.
+  if (L->empty())
+    return false;
----------------
as it is currently, we should only get to this point for outer loops, right? Then this could be an assert.


================
Comment at: lib/Transforms/Vectorize/VPlan.cpp:403
+          isa<UnreachableInst>(LastBB->getTerminator())) &&
          "Expected VPlan CFG to terminate with unreachable");
+  assert((!EnableVPlanNativePath || isa<BranchInst>(LastBB->getTerminator())) &&
----------------
This message needs to be updated I think. Something like 'Expected InnerLoop VPlan CFG to terminate with unreachable')


Repository:
  rL LLVM

https://reviews.llvm.org/D50820