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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:166
+      Instruction *I = cast<Instruction>(
+          cast<VPReplicateRecipe>(SinkCandidate)->getUnderlyingValue());
       auto *Clone =
----------------
Ayal wrote:
> fhahn wrote:
> > 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.
> Ahh, right, that use was indeed outside the lambda.
> 
> > 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.
> 
> Yeah, the filtering of getDefiningRecipe() would then be required twice - both when initializing the worklist and when augmenting it inside the loop, but clearly only recipes/non-live-in-VPValues are candidates for sinking, and doing so would clarify the SinkCandidateValue versus SinkCandidate ambiguity.
> Yeah, the filtering of getDefiningRecipe() would then be required twice - both when initializing the worklist and when augmenting it inside the loop, but clearly only recipes/non-live-in-VPValues are candidates for sinking, and doing so would clarify the SinkCandidateValue versus SinkCandidate ambiguity.


I updated the code to use VPRecipeBase for the worklist in the committed version, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133760