[PATCH] D133760: [VPlan] Support sinking VPScalarIVStepsRecipe.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9577
+    EndPart = StartPart + 1;
+  }
+
----------------
Ayal wrote:
> nit: should probably have a utility for all interested users to set up {StartPart, EndPart} from a given State.
> 
> May be worth trying to unify this handling of VF=1 within buildScalarSteps (TODO?).
> May be worth trying to unify this handling of VF=1 within buildScalarSteps (TODO?).

That was straight-forward, as `buildScalarSteps` already handles `VF = 1` the same way as the code below: e25ed058bc23


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:125
 
   // Try to sink each replicate recipe in the worklist.
   while (!WorkList.empty()) {
----------------
Ayal wrote:
> replicate [or scalar IV steps] recipe
Adjusted, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:136
+      if (!RepR || RepR->isUniform() || RepR->mayHaveSideEffects() ||
+          RepR->mayReadOrWriteMemory())
+        continue;
----------------
Ayal wrote:
> Worth having a "canSinkScalar()" in RecipeBase, or VPValue?
I think for now it would be better to keep it here as its only user.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:149
     // SinkCandidate. At the moment, we identify such UAV's by looking for the
     // address operands of widened memory recipes.
+    auto CanSinkWithUser = [SinkTo, &NeedsDuplicating, C](VPUser *U) {
----------------
Ayal wrote:
> Can we check directly if onlyFirstLaneUsed()?
I've put up D133760 to address that.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:168
+      Instruction *I = cast<Instruction>(
+          cast<VPReplicateRecipe>(SinkCandidate)->getUnderlyingValue());
       auto *Clone =
----------------
Ayal wrote:
> Could ScalarIVSteps also feed a user using only first lane?
> Conservatively check and set NeedsDuplicating only for RecplicateRecipes?
Yes it could after changing to use `onlyFirstLaneUsed`, I added a `continue` for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133760