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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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.
----------------
Ayal wrote:
> 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.
This code is now completely gone.


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


================
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()) {
----------------
Ayal wrote:
> change independent of this patch?
Done in 5b00d13c0071.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:335
     State->CFG.PrevBB = NewBB;
+    State->Builder.SetInsertPoint(NewBB, NewBB->begin());
   } else if (PrevVPBB && /* A */
----------------
Ayal wrote:
> 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?
Should be adjusted in the current version, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1054
   }
+
+  for (VPExitValue *U : ExitValues) {
----------------
Ayal wrote:
> Set the insertion point of State's builder here?
Done in the latest version (at the new place this is called)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1107
 
+void VPlan::addExitValue(PHINode *PN, VPValue *V) {
+  ExitValues.push_back(new VPExitValue(PN, V));
----------------
Ayal wrote:
> Should this check for duplicates, as in getOrAdd?
I think this should never be called multiple times with the same `PN`. Added an assertion.


================
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 {
----------------
Ayal wrote:
> nit: "recipe"
Fixed, thanks!


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537