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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me, thnks!
Adding a minor optional nit.



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:154
+      NeedsDuplicating = UI->onlyFirstLaneUsed(SinkCandidateValue);
+      // We only know how to duplicate VPRecipeRecipes for now.
+      return NeedsDuplicating && isa<VPReplicateRecipe>(SinkCandidate);
----------------
It does look better to return false and claim !CanSinkWithUser when duplication is needed for a non replicate recipe, leading to capturing both SinkCandidate and SinkCandidateValue...


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:166
+      Instruction *I = cast<Instruction>(
+          cast<VPReplicateRecipe>(SinkCandidate)->getUnderlyingValue());
       auto *Clone =
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133760