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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
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) {
----------------
Ayal wrote:
> 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.
updated the comment, thanks!


================
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());
----------------
Ayal wrote:
> 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.)
Updated to use a `dyn_cast`, so the code does not need to rely on what kinds of plans are passed in.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100258