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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.

Looks good to me, and raises some thoughts ;-)



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3861
         Incoming, Idx, "vector.recur.extract.for.phi");
   } else if (UF > 1)
     // When loop is unrolled without vectorizing, initialize
----------------
nit (Independent of this patch): else block deserves parentheses considering 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());
----------------
nit: "only user should be FirstOrderRecurrenceSplice VPInstruction" >>
"Recurrence Phi must have a single user: FirstOrderRecurrenceSplice"?


================
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;
----------------
nit: worth asserting opcode is FirstOrderRecurrenceSplice ?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3887
+    if (auto *LiveOut = dyn_cast<VPLiveOut>(U))
+      LiveOuts.push_back(LiveOut);
+
----------------
(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?


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


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3893
+    LCSSAPhi->addIncoming(ExtractForPhiUsedOutsideLoop, LoopMiddleBlock);
+    State.Plan->removeLiveOut(LCSSAPhi);
   }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147472