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

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked 2 inline comments as done.
fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1279
+
+  void setIsPredicated(bool New = true) { IsPredicated = New; }
 };
----------------
gilr wrote:
> Allowing to reset IsPredicated seems a bit fragile, as alsoPack is computed on contruction based on it. Since this patch only requires setting IsPredicated, perhaps better to have `setIsPredicated() { IsPredicated = true; }`. Might be worth adding a comment here that setting IsPredicated late is consistent with alsoPack being initialized to false as all users of the sunk recipes reside within the replicated region so no packing is required.
Agreed! I added a comment and an assertion.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258