[PATCH] D42447: [LV][VPlan] Detect outer loops for explicit vectorization.

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2319
+// LoopVectorizeHints). This will be fixed in the future when the native IR
+// representation for pragma 'omp simd' is introduced.
+static bool isExplicitVecOuterLoop(Loop *OuterLp,
----------------
Sorry, this is my fault, as both were done separately. We discussed adding one more metadata info to mean {"forced"/"hint"} but ended up never doing it. It should be simple to fix this, I think, just need to make sure we change all the tests correctly to what they're supposed to mean.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:2363
       V.push_back(&L);
-    return;
+      // TODO: Collect inner loops inside marked outer loops in case
+      // vectorization fails for the outer loop. Do not invoke
----------------
What's the complexity of this analysis? We'll be adding a lot of those, repeatedly, no?

If I understand correctly, `containsIrreducibleCFG` is not that simple, in addition to the traversal. Not calling it unnecessarily would be a nice thing to have up front.

How complex would it be to create the inherited attribute?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5138
+  }
+
+  Value *CondOp0 = LatchCmp->getOperand(0);
----------------
nit. I'd remove this space, as both blocks are regarding (3)


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7642
+  if (!OrigLoop->empty()) {
+    assert(EnableVPlanNativePath && "VPlan-native path is not enabled.");
+    assert(UserVF && "Expected UserVF for outer loop vectorization.");
----------------
I wouldn't make this an assert, just a debug message and return


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7647
+    buildVPlans(UserVF, UserVF);
+    // TODO: DEBUG(printPlans(dbgs()));
+
----------------
left over?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7650
+    // TODO: VPlan-based Predication.
+    // TODO: VPlan-based Cost Modeling.
+
----------------
is this really the place to do predication and cost modelling?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:8227
+    // VPlanHCFGBuilder HCFGBuilder(OrigLoop, LI, *Plan.get());
+    // HCFGBuilder.buildHierarchicalCFG();
+    DEBUG(dbgs() << "LV: TODO: Build hierarchical CFG for outer loop.\n");
----------------
Isn't `containsIrreducibleCFG` depending on this to work?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:8228
+    // HCFGBuilder.buildHierarchicalCFG();
+    DEBUG(dbgs() << "LV: TODO: Build hierarchical CFG for outer loop.\n");
+
----------------
Better not to have TODOs as debug messages.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:8642
+
+     // TODO: It will be set to true when we introduce codegen support.
+     return false;
----------------
I'm really uncomfortable with all these temporary code blocks that don't do anything...

They're really just hijacking the existing infrastructure without implementing as a VPlan.

I really thought the whole point of VPlans was that you wouldn't need to hack-it-up like we used to in the old vectoriser...


https://reviews.llvm.org/D42447