[PATCH] D133760: [VPlan] Support sinking VPScalarIVStepsRecipe.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:52
   case VPBranchOnMaskSC:
+  case VPScalarIVStepsSC:
     return false;
----------------
Ayal wrote:
> nit: indicating that VPScalarIVStepsSC may not write or read from memory is independent of this patch but is exercised by it.
Submitted separately in 3c5f07349fd8 with unit test coverage added in cf28e6b2f117


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:141
+    if (SinkCandidate->getParent() == SinkTo)
+      continue;
 
----------------
Ayal wrote:
> nit: checking if SinkCandidate already resides in SinkTo is independent of its recipe type and can be combined with above `if (!SinkCandidate || ...)` checks, as suggested above?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:166
+      Instruction *I = cast<Instruction>(
+          cast<VPReplicateRecipe>(SinkCandidate)->getUnderlyingValue());
       auto *Clone =
----------------
Ayal wrote:
> Still use (uncaptured) SinkCandidate in addition to SinkCandidateValue?
I might be missing something here, but this is use is outside the lambda so doesn't need capturing?

We could only use `SinkCandidate` and add `VPRecipeBase*` to to worklist instead of `VPValue*`, but then we would need to filter out live ins in multiple places where we add to the worklist.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133760