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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
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()) {
----------------
fhahn wrote:
> Ayal wrote:
> > change independent of this patch?
> Done in 5b00d13c0071.
While we're here, first set Header then set its name?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10683
+        // Update exit values for LCSSA phis in the exit block for the epilogue
+        // loop.
+        VPBasicBlock *MiddleVPBB =
----------------
How do the new exit values created by addUsersInExitBlock() below differ from the existing exit values of BestEpiPlan they replace? Both wrap the same LCSSA phis and presumably use the same VPValue from within BestEpiPlan. When called to fix their phi, they should update distinct incoming values corresponding to distinct middle blocks, but those are independent of the exit values themselves - that's why Plan is provided to fixPhi()?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:10692
+                            DeadInstructions);
+
         LVP.executePlan(EPI.EpilogueVF, EPI.EpilogueUF, BestEpiPlan, EpilogILV,
----------------
This processLoop() method is becoming 388 LOC long... any change to it should consider refactoring?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.cpp:1087
+  for (VPExitValue *EV : ExitValues) {
+    O << "Live-out ";
+    EV->getPhi()->printAsOperand(O);
----------------
Perhaps call them VPLiveOut/VPLiveOutIR/VPLiveOutUser/VPLiveOutIRUser rather than VPExitValue, given how they are printed, and to complement live-in IR Values which are represented via VPDef-less VPValues supporting getLiveInIRValue()?

assert/verify/note somewhere that VPExitValue is assumed to have a single operand?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:685
+
+  PHINode *getPhi() { return Phi; }
+};
----------------
const?
(Currently used for printing only.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2629
       addVPValue(V);
-    return getVPValue(V);
+    return getVPValue(V, OverrideAllowed);
   }
----------------
Change related to this patch?
Can a test demonstrate its need?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123537