[PATCH] D120828: [LV] Create & use VPScalarIVSteps for all scalar users.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9565
+  // required to be loop-invariant
+  auto CreateStepValue = [&](const SCEV *Step) -> Value * {
+    if (SE.isSCEVable(IV->getType())) {
----------------
Ayal wrote:
> We're losing the assertion for loop-invariance of step here. Worth retaining, somewhere else perhaps?
Good point, I added it to the place where VPWidenIntOrFpInductionRecipe is created.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:387
 
 void VPlanTransforms::optimizeInductions(VPlan &Plan, ScalarEvolution &SE) {
   SmallVector<VPRecipeBase *> ToRemove;
----------------
Ayal wrote:
> Update documentation of optimizeInductions()?
> This is an optional performance optimization, to save extracts?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp:427
+          continue;
+        if (R->onlyScalarsUsed(IV) || R->onlyFirstLaneUsed(IV))
+          R->setOperand(I, Steps);
----------------
Ayal wrote:
> Checking if R->onlyFirstLaneUsed(IV) is redundant - suffice to check if onlyScalarsUsed(IV) as the latter covers the former?
> 
> First check if only scalars are used, then if so go look for the operand to replace, preferably using some "findOperand" lambda? If not some "replaceAllScalarUsesWith(IV, Steps)"
> Checking if R->onlyFirstLaneUsed(IV) is redundant - suffice to check if onlyScalarsUsed(IV) as the latter covers the former?

Done!

> First check if only scalars are used, then if so go look for the operand to replace, preferably using some "findOperand" lambda? If not some "replaceAllScalarUsesWith(IV, Steps)"

If there are no vector users, we can directly keep using replaceAllUsesWith. For the other case, I kept the loop, but with first checking if only scalars are used by the recipe.

I kept the loop as is for now, as it seems quite specialized at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120828