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

[Gil Rapaport via Phabricator via llvm-commits]

gilr added a comment.

Very nice step forward!!



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1279
+
+  void setIsPredicated(bool New = true) { IsPredicated = New; }
 };
----------------
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.


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



================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:128
+              auto *UI = dyn_cast<VPRecipeBase>(U);
+              return UI && UI->getParent() != VPBB;
+            }))
----------------
Should be `return !UI || UI->getParent() != VPBB`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:133
+        Current->moveBefore(*RepR->getParent(), RepR->getParent()->begin());
+        Current->setIsPredicated();
+        WorkList.insert(Current->op_begin(), Current->op_end());
----------------
Would be good to leave a comment here explaining that setting IsPredicated is done in favor of the legacy sinkScalarOperands() which still follows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258