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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4604
+      // If the instruction is already in PredBB, check if we can sink its
+      // operands.
+      if (I->getParent() == PredBB) {
----------------
Ayal wrote:
> Perhaps worth noting that VPlan's sinkScalarOperands() was responsible for sinking the instruction into PredBB in this case, but may have failed to sink its (direct or indirect) operands, which we try again here.
I updated the comment to reference VPlanTransforms::sinkScalarOperands and the interaction here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:127
+
+        Current->moveBefore(*RepR->getParent(), RepR->getParent()->begin());
+        WorkList.insert(Current->op_begin(), Current->op_end());
----------------
Ayal wrote:
> Note that in general, insertion point should be after all phi's, but none are expected in VPBB; right?
Yes, there cannot be any PHI recipes in the block, but nevertheless it won't hurt to get the first non-phi recipe, in case that changes in the future. Updated the code.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:108
+  bool Changed = false;
+  for (VPBlockBase *Base : RPOT) {
+    auto *VPBB = dyn_cast<VPBasicBlock>(Base);
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258