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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:166
+      Instruction *I = cast<Instruction>(
+          cast<VPReplicateRecipe>(SinkCandidate)->getUnderlyingValue());
       auto *Clone =
----------------
fhahn wrote:
> 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!
> I updated the code to use VPRecipeBase for the worklist in the committed version, thanks!

Very well!

There's still some Recipe/VPValue ambiguity left in multiple getVPSingleValue()'s (deserves an explicit comment or assert that only single valued recipes are handled?). This stems from handling not only Replicate Recipes but also sinking VPScalarIVStepsRecipes. Which may suggest introducing a VPSingleValueRecipe base class... although conceptually multiple value recipes could also potentially be sunk in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133760