[PATCH] D147472: [VPlan] Use VPLiveOut to update FOR live-out users.

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 6 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3882
+  assert(PhiR->getNumUsers() == 1 &&
+         "only user should be FirstOrderRecurrenceSplice VPInstruction");
+  auto RecurSplice = cast<VPInstruction>(*PhiR->user_begin());
----------------
Ayal wrote:
> nit: "only user should be FirstOrderRecurrenceSplice VPInstruction" >>
> "Recurrence Phi must have a single user: FirstOrderRecurrenceSplice"?
adjusted thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3883
+         "only user should be FirstOrderRecurrenceSplice VPInstruction");
+  auto RecurSplice = cast<VPInstruction>(*PhiR->user_begin());
+  SmallVector<VPLiveOut *> LiveOuts;
----------------
Ayal wrote:
> nit: worth asserting opcode is FirstOrderRecurrenceSplice ?
Added the check to the assert.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3887
+    if (auto *LiveOut = dyn_cast<VPLiveOut>(U))
+      LiveOuts.push_back(LiveOut);
+
----------------
Ayal wrote:
> (Independent of this patch:) Do multiple live-out users feed multiple LCSSAPhi's, should they all feed the same LCSSAPhi, are there tests covering multiple live-out users?
They feed different LCSSA phis, the test for that is `llvm/test/Transforms/LoopVectorize/pr36983.ll`.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3890
+  for (VPLiveOut *LiveOut : LiveOuts) {
+    assert(!Cost->requiresScalarEpilogue(VF));
+    PHINode *LCSSAPhi = LiveOut->getPhi();
----------------
Ayal wrote:
> nit: add message to assert.
Thanks, I added a message to the assert.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3893
+    LCSSAPhi->addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
+    State.Plan->removeLiveOut(LCSSAPhi);
   }
----------------
Ayal wrote:
> TODO (Independent of this patch): `LiveOuts` is presumably needed only for delayed destruction of VPLiveOuts, as adding an incoming IR Value to LCSSAPhi  should not interfere with traversing the VPUsers of RecurSplice. Why are VPLiveOut VPUsers destroyed here, rather than during the destruction of VPlan, along with all other VPUsers?
> This applies to the other cases of removeLiveOut() as well.
At the moment, logic later relies on any live-outs that cannot fix themselves to be removed  so they are excluded from the handling below

```
  // Fix LCSSA phis not already fixed earlier. Extracts may need to be generated
  // in the exit block, so update the builder.
  State.Builder.SetInsertPoint(State.CFG.ExitBB->getFirstNonPHI());
  for (const auto &KV : Plan.getLiveOuts())
    KV.second->fixPhi(Plan, State);
```.

Once live-outs of FOR & IVs can be handled completely in VPlan, no removal should be necessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147472