[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:132
+    for (VPUser *U : SinkCandidate->users()) {
+      auto *UI = dyn_cast<VPRecipeBase>(U);
+      if (!UI)
----------------
Ayal wrote:
> dyn_cast U directly to VPReplicateRecipe?
The way the various IDs are set up, we need a cast from VPUser -> VPRecipeBase and then a cast from VPRecipeBase -> VPReplicateRecipe. It would be possible to add a `classof` implementation to do so. Not sure if there's a general way to do so for all recipes in a single place.

But this is not directly relevant any longer, because the code is gone in the current version of the patch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:141
+    }
+    if (!SinkTo || SinkCandidate->getParent() == SinkTo ||
         SinkCandidate->mayHaveSideEffects() ||
----------------
Ayal wrote:
> 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.
Updated to record it in the worklist directly.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:171
+      auto *Clone =
+          new VPReplicateRecipe(I, SinkCandidate->operands(), true, false);
+
----------------
Ayal wrote:
> Should Clone and SinkCandidate have distinct names, e.g., append ".cloned" to the former somehow?
Unfortunately this is not possible at the moment, because VPValues do not have associated names that can be changed directly. I added a todo.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104254