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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:132
+    for (VPUser *U : SinkCandidate->users()) {
+      auto *UI = dyn_cast<VPRecipeBase>(U);
+      if (!UI)
----------------
dyn_cast U directly to VPReplicateRecipe?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:141
+    }
+    if (!SinkTo || SinkCandidate->getParent() == SinkTo ||
         SinkCandidate->mayHaveSideEffects() ||
----------------
Can assert(SinkTo && "...") - SinkCandidate surely has a ReplicateRecipe user, which got it into WorkList. As commented above, this user could be recorded too in WorkList.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:160
+
+                 if (IsGEP && isa<VPWidenMemoryInstructionRecipe>(UI)) {
+                   NeedsDuplicating = true;
----------------
Check if SinkCandidate == UI->getAddr() instead of IsGEP? 


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:171
+      auto *Clone =
+          new VPReplicateRecipe(I, SinkCandidate->operands(), true, false);
+
----------------
Should Clone and SinkCandidate have distinct names, e.g., append ".cloned" to the former somehow?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:178
+        auto *UI = dyn_cast<VPRecipeBase>(U);
+        if (!UI || UI->getParent() == SinkTo)
+          continue;
----------------
At this point, all users must be recipes; can do a static cast?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104254