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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks for the changes!
Few remaining nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4611
+      // failed to sink the instructions due to current limitations. Try to sink
+      // the scalar instruction here.
+      if (I->getParent() == PredBB) {
----------------
To avoid confusion may help to state more accurately. VPlan's sinkScalarOperands() succeeded in sinking the scalar instruction `I`, hence it appears in PredBB; but it may have failed to sink `I`'s operands (recursively), which we try (again) here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:128
+    // sinking. Therefor the destination block for sinking must match the block
+    // containing the first user.
+    auto *FirstUser = cast<VPRecipeBase>(*SinkCandidate->user_begin());
----------------
Therefor[e].

Note that SinkCandidate must have at-least one user.
Should cast<recipe>(first user) be dyn_cast (as below), or else drop checking if it's null?
(Note that sinkScalarOperands should not encounter 'native's block users, so it should be ok to cast the first user into a recipe.)


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:132
+      continue;
+    VPBasicBlock *SinkTo = cast<VPBasicBlock>(FirstUser->getParent());
+    if (SinkCandidate->getParent() == SinkTo ||
----------------
Is the cast<VPBasicBlock> needed for SinkTo? (due to const(?))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258