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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline 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(),
----------------
Ayal wrote:
> 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.
Thanks, updated the wording.


================
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);
----------------
Ayal wrote:
> Might be clearer to give this lambda, which checks two things, a meaningful name?
I renamed it to `CanSinkWithUser ` and flipped the return values accordingly. This seems more natural.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:161
+      SmallVector<VPUser *, 4> Users(SinkCandidate->user_begin(),
+                                     SinkCandidate->user_end());
+      for (auto *U : Users) {
----------------
Ayal wrote:
> Users are captured as their traversal may modify them. Can alternatively use that iterator which tolerates modification.
Unfortunately the underlying storage for users here is `SmallVector`, which does not tolerate modifications AFAICT.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:116
         continue;
       WorkList.insert(RepR->op_begin(), RepR->op_end());
     }
----------------
Ayal wrote:
> 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.
Done in 7359450e6a05


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104254