This is an archive of the discontinued LLVM Phabricator instance.

[LV] Create & use VPScalarIVSteps for all scalar users.
ClosedPublic

Authored by fhahn on Mar 2 2022, 9:06 AM.

Details

Summary

This patch is a follow-up to D115953. It updates optimizeInductions
to also introduce new VPScalarIVStepsRecipes if an IV has both vector
and scalar uses.

It updates all uses that only need scalar values to use the newly
created recipe for the scalar steps.

This completes untangling of VPWidenIntOrFpInductionRecipe
code-generation. Now the recipe *only* creates the widened vector
values, as it says on the tin.

The code to genereate IR has been moved directly to
VPWidenIntOrFpInductionRecipe::execute.

Note that the recipe has been updated to hold a reference to
ScalarEvolution, which is needed to expand the step, until we can place
the corresponding SCEV expansion in the pre-header.

Depends on D120827.

Diff Detail

Event Timeline

fhahn created this revision.Mar 2 2022, 9:06 AM
fhahn requested review of this revision.Mar 2 2022, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 9:06 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added a comment.Mar 8 2022, 9:32 AM

This nicely removes from ILV both widenIntOrFpInduction() along with createVectorIntOrFpInductionPHI() which it calls, inlining them into VPlan's VPWidenIntOrFpInductionRecipe::execute(), while postponing buildScalarSteps() for all potential scalar users of the vectorized IV to a VPlan2VPlan transformation/optimization.

Is VPWidenIntOrFpInductionRecipe still created when there are only scalar users, and discarded later by useless recipe removal?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9565

We're losing the assertion for loop-invariance of step here. Worth retaining, somewhere else perhaps?

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
387

Update documentation of optimizeInductions()?
This is an optional performance optimization, to save extracts?

427

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)"

fhahn updated this revision to Diff 414059.Mar 9 2022, 4:07 AM
fhahn marked 3 inline comments as done.

Address latest comments, thanks!

This nicely removes from ILV both widenIntOrFpInduction() along with createVectorIntOrFpInductionPHI() which it calls, inlining them into VPlan's VPWidenIntOrFpInductionRecipe::execute(), while postponing buildScalarSteps() for all potential scalar users of the vectorized IV to a VPlan2VPlan transformation/optimization.

Is VPWidenIntOrFpInductionRecipe still created when there are only scalar users, and discarded later by useless recipe removal?

Yes, VPWidenIntOrFpInductionRecipe are created unconditionally and optimzed later.

fhahn updated this revision to Diff 414060.Mar 9 2022, 4:07 AM

Fix formatting

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9565

Good point, I added it to the place where VPWidenIntOrFpInductionRecipe is created.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
387

Updated, thanks!

427

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.

Ayal added inline comments.Mar 9 2022, 11:22 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9565

Very good!

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
424

'continue' on same line?

Given that we're iterating over all users below to find those that use scalars, perhaps compute if a vector IV and scalar IV are needed, removing needsVectorIV() and needsScalarIV()? Along with what they entail...

fhahn updated this revision to Diff 414194.Mar 9 2022, 12:58 PM

Fix continue.

fhahn marked an inline comment as done.Mar 9 2022, 1:00 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
424

'continue' on same line?

Fixed, thanks!

Given that we're iterating over all users below to find those that use scalars, perhaps compute if a vector IV and scalar IV are needed, removing needsVectorIV() and needsScalarIV()? Along with what they entail...

I think that's a good follow-up. There's another user of needsVectorIV outside the function, so I think it would be good to make this change separately.

Ayal accepted this revision.Mar 10 2022, 6:53 AM

Ship it!
Would be good to follow-up with cleaning-up of needsScalarIV/needsVectorIV.

This revision is now accepted and ready to land.Mar 10 2022, 6:53 AM
This revision was landed with ongoing or failed builds.Mar 13 2022, 10:15 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.