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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:127
+
+        Current->moveBefore(*RepR->getParent(), RepR->getParent()->begin());
+        WorkList.insert(Current->op_begin(), Current->op_end());
----------------
Note that in general, insertion point should be after all phi's, but none are expected in VPBB; right?


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258