[PATCH] D97712: [VPlan] Clone original VPlan and specialize for each VF (WIP).

[Andrei Elovikov via Phabricator via llvm-commits]

a.elovikov added a comment.

I didn't recognize that it's still WIP until some point, so some nitpicking comments aren't very useful probably...



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h:281
 
+  VPlanPtr clone(VPlan &OriginalPlan);
+
----------------
Are we somehow limited that the argument must be an OriginalPlan or can it be some Plan? Specifically, what if I want to try some optimization on already cloned VPlan such that I'm not sure if it's profitable or not, so I want to create a clone of the Plan instead of transforming in place. Would I be able to use that `clone` method?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8738
+
+    VPBasicBlock *OriginalVPBB = Base->getEntryBasicBlock();
+    VPBasicBlock *VPBB = cast<VPBasicBlock>(Old2NewBlocks[OriginalVPBB]);
----------------
Why don't we use `cast` similar to line 8721? It's also not clear why we have early continue above, but it might actually have the same answer.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8746
+      VPRecipeBase *Ingredient = &*I++;
+      auto RemapOperands = [&Old2New, &RemapAfter, &Plan](VPRecipeBase *U) {
+        if (isa<VPWidenPHIRecipe>(U)) {
----------------
nit: I'd move it out of the loop.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8780
+    for (unsigned I = 0, E = U->getNumOperands(); I != E; ++I) {
+      VPValue *OldOp = U->getOperand(I);
+      VPValue *NewOp = nullptr;
----------------
That's mostly copy of the lambda above. Can we unify them somehow?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8888
       if (DeadInstructions.contains(Instr) || isa<DbgInfoIntrinsic>(Instr)) {
+        for (VPUser *U :
+             make_early_inc_range(Ingredient->getVPValue()->users())) {
----------------
I think it deserves a dedicated comment, especially because it's surprising to see new recipes created inside the block of code starting with the comment "Remove dead instrutions."

I think that it would be beneficial to address this even in the WIP stage of the patch and not during the final polishing.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:585
+  static inline bool classof(const VPUser *D) {
+    // All VPDefs are also VPRecipeBases.
+    return true;
----------------
Missed in the copy-paste?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97712/new/

https://reviews.llvm.org/D97712