[PATCH] D123537: [VPlan] Model first exit values in VPlan.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Summary deserves updating.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:8735
+
+  // Introduce recipes modeling the exit values. They represent LCSSA phis and
+  // must be added to the beginning of the exit/middle block.
----------------
Comment needs updating: exit values are now represented as VPUsers rather than recipes, they belong to VPlan - 'residing' outside any of VPlan's VPBasicBlock, rather than be added to the beginning of exit/middle block.
No need to set insertion point.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10667
+        VPRegionBlock *VectorLoop = BestEpiPlan.getVectorLoopRegion();
+        VectorLoop->getEntryBasicBlock()->setName("vec.epilog.vector.body");
 
----------------
change independent of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10671
         // updated before vectorising the epilogue loop.
-        VPBasicBlock *Header =
-            BestEpiPlan.getVectorLoopRegion()->getEntryBasicBlock();
+        VPBasicBlock *Header = VectorLoop->getEntryBasicBlock();
         for (VPRecipeBase &R : Header->phis()) {
----------------
change independent of this patch?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:335
     State->CFG.PrevBB = NewBB;
+    State->Builder.SetInsertPoint(NewBB, NewBB->begin());
   } else if (PrevVPBB && /* A */
----------------
Better to set the insertion point to middle block when calling fixPhi's, rather than here, noting that the latter may need to generate extracts there?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1054
   }
+
+  for (VPExitValue *U : ExitValues) {
----------------
Set the insertion point of State's builder here?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1107
 
+void VPlan::addExitValue(PHINode *PN, VPValue *V) {
+  ExitValues.push_back(new VPExitValue(PN, V));
----------------
Should this check for duplicates, as in getOrAdd?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:678
+
+  /// Returns true if the recipe uses scalars of operand \p Op.
+  bool usesScalars(const VPValue *Op) const override {
----------------
nit: "recipe"


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2524
       VPValue DummyValue;
+      clearExitValues();
       for (VPBlockBase *Block : depth_first(Entry))
----------------
Can clear exit values regardless if Entry or not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537