[PATCH] D44338: [LV][VPlan] Build plain CFG with simple VPInstructions for outer loops.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a reviewer: aprantl.
fhahn added a subscriber: aprantl.
fhahn added a comment.

Thanks Diego! I left some small comments inline, I think overall it looks quite good now. One thing worth discussing briefly before this goes in may be what the plan for dealing with debug info will be with VPlan. Adding @aprantl  in case he has some thoughts.



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:73
+  public:
+    /// \brief Creates a new insertion point which doesn't point to anything.
+    VPInsertPoint() = default;
----------------
There's been a few commits removing \brief from the codebase (e.g. D46290) recently, could you remove \brief from this patch, it should not be needed as we use AUTOBRIEF.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationPlanner.h:150
+    VPBuilder &Builder;
+    // TODO: AssertingVH<VPBasicBlock> Block;
+    VPBasicBlock* Block;
----------------
What does this comment mean?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:133
+bool PlainCFGBuilder::isExternalDef(Value *Val) {
+  // All the Values that are not Instructions are considered external
+  // definitions for now.
----------------
Could we use a similar (simpler) logic to what @hsaito  used in D46302 here? Like  Instr->getParent() strictly dominates the pre header?


================
Comment at: lib/Transforms/Vectorize/VPlanHCFGBuilder.cpp:163
+// Create a new VPValue or retrieve an existing one for the Instruction's
+// operand \p IROp. This function must only be used to create/retrieve VPValues
+// for *Instruction's operands* and not to create regular VPInstruction's. For
----------------
nit: IRVal as it is a value?


https://reviews.llvm.org/D44338