[PATCH] D104254: [VPlan] Support sinking recipes with uniform users outside sink target.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

This looks good to me, thanks for following-up! Adding some last, minor comments.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:136
+    // need to duplicate SinkCandidate. At the moment, we only check for GEPs as
+    // sink candidates, used by widened memory recipes.
+    if (any_of(SinkCandidate->users(),
----------------
Update comment? By 'uniform' we mean 'uniform-after-vectorization', i.e., only first lane is used. (Truly uniforms - all lanes have same value - are filtered above to avoid sinking.) At the moment, we identify such UAV's by looking for the address operands of widened memory recipes.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:138
+    if (any_of(SinkCandidate->users(),
+               [SinkTo, &NeedsDuplicating, SinkCandidate](VPUser *U) {
+                 auto *UI = dyn_cast<VPRecipeBase>(U);
----------------
Might be clearer to give this lambda, which checks two things, a meaningful name?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:161
+      SmallVector<VPUser *, 4> Users(SinkCandidate->user_begin(),
+                                     SinkCandidate->user_end());
+      for (auto *U : Users) {
----------------
Users are captured as their traversal may modify them. Can alternatively use that iterator which tolerates modification.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:167
+
+        for (unsigned I = 0; I != UI->getNumOperands(); I++) {
+          if (UI->getOperand(I) != SinkCandidate)
----------------
`I` is already being used above. Rename?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:116
         continue;
       WorkList.insert(RepR->op_begin(), RepR->op_end());
     }
----------------
Ayal wrote:
> Would it simplify the processing below if WorkList contains pairs of RepR along with each of its operands, as a first user thereof, saving the search for it, i.e., for SinkTo?
Thanks for following-up! This refactoring could potentially be committed separately, as an NFC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104254