[PATCH] D100258: [VPlan] Add first VPlan version of sinkScalarOperands.

[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, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:127
+    // All users of SinkCandidate must be in the same block in order to perform
+    // sinking. Therefor the destination block for sinking must match the block
+    // containing the first user.
----------------
"Therefor the" >> "Therefore the"


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:108
+  bool Changed = false;
+  for (VPBlockBase *Base : RPOT) {
+    auto *VPBB = dyn_cast<VPBasicBlock>(Base);
----------------
fhahn wrote:
> Ayal wrote:
> > fhahn wrote:
> > > gilr wrote:
> > > > Traversing the VPlan in this case doesn't require RPOT, right? can be done with a more efficient recursive scan?
> > > > Simpler to first collect all seeds into a worklist, saving the need to make early increments? If so, can alternatively be done by RecipeBuilder during VPlan construction.
> > > > 
> > > That's a good point, the traversal order should not matter. I updated it to use depth-first.
> > > 
> > > > Simpler to first collect all seeds into a worklist, saving the need to make early increments? If so, can alternatively be done by RecipeBuilder during VPlan construction.
> > > 
> > > It may be simpler, but would introduce coupling with the recipe builder and I am not sure that's desired at this point? It seems like it may limit composability of VPlan transforms (e.g. if another transform introduces new predicated recipes, it may have to update the list of seed instructions in the builder or somewhere else)
> > > Simpler to first collect all seeds into a worklist, saving the need to make early increments?
> > 
> > I.e., instead of operating in-place using make_early_inc_range(), and somewhat similar to the original sinkScalarOperand()'s use of `PredicatedInstructions`, would it be better to first enlist all candidates into `WorkList`, and then loop over it? VPBB can be set to the parent of the first user, which all other users must agree with via the any_of(). In any case, better reuse VPBB instead of RepR->getParent(); and choose more meaningful names than Current, RepR and VPBB to deal with sinking a source Def into the destination basic block where all its Users reside.
> > 
> > Interesting to see that `InstsToReanalyze` is not needed, presumably because the last user to sink would always trigger the potential sinking of its def? Probably worth a note.
> I updated the code to first collect all instructions in the initial worklist and then process them separately.
> 
> I also adjusted the variable names.
> 
> It looks like `InstsToReanalyze` is not required for any existing test cases, even for the implementation in LV. I tried to construct cases, but if a recipe is used by multiple recipes that are to be sunk, the one that is used multiple times does not get scalarized. I think it's not needed, because if we sink a recipe, its operands get added to the worklist again and the only way it enables further sinking is that there must be a def-use relation between to newly sunk recipe and the one we need to re-try. 
Interesting; might be worth removing the presumably redundant `InstsToReanalyze`, in a separate patch.
In any case, VPlan's sinkScalarOperands() should be enhanced to retire the original sinkScalarOperands() altogether.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258