This is an archive of the discontinued LLVM Phabricator instance.

Scalarize calls to masked functions in LV
ClosedPublic

Authored by huntergr on Sep 22 2022, 2:16 AM.

Details

Summary

This patch adds support for scalarizing calls to a function when there is a vector variant that cannot be used, either because there isn't a masked variant or because the cost model indicated a VF with out a masked variant was better.

Diff Detail

Event Timeline

huntergr created this revision.Sep 22 2022, 2:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 22 2022, 2:16 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
huntergr requested review of this revision.Sep 22 2022, 2:16 AM
mgabka added a subscriber: mgabka.Sep 26 2022, 7:14 AM
Matt added a subscriber: Matt.Feb 2 2023, 1:15 PM
paulwalker-arm added inline comments.
llvm/include/llvm/Analysis/VectorUtils.h
129 ↗(On Diff #462118)

Perhaps have this return Optional<unsigned> to give people the option to use either if (isMasked()) or if (auto Pos = getParamIndexForMask()) depending on what works best for them?

It just seems bad to force people to traverse the shape parameters multiple time and means getParamIndexForOptionalMask can be removed.

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp
1137–1140

Is it possible to use any_of here?

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

Given we cannot scalarise scalable vectors should this return InvalidCost somewhere? Or perhaps blockCanBePredicated should return false? Perhaps I'm being paranoid but the code should be resilient to the case of somebody adding a bunch of scalable vector TLI mapping before all the LoopVectorize support is finished. Or do tests already exist for scalable vectors that show it does not matter?

huntergr updated this revision to Diff 500802.Feb 27 2023, 8:21 AM

Rebased, amended patch per review comments.

huntergr marked 2 inline comments as done.Feb 27 2023, 8:27 AM
huntergr added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
3583

So Cost should already be Invalid from the call to getScalarizationOverhead above, but I've added in another check here just to be sure -- while we might one day implement scalarization for a subset of operations if there's a need for it, I don't think this will be one of them due to the standard AArch64 PCS.

I was going to add a test for this but can't find a nice way of doing so yet with the 3rd patch in the series implementing full mask support -- it will probably have to wait until we remove the restriction on needing a variant.

paulwalker-arm accepted this revision.Mar 1 2023, 9:31 AM
This revision is now accepted and ready to land.Mar 1 2023, 9:31 AM
This revision was landed with ongoing or failed builds.Mar 3 2023, 7:39 AM
This revision was automatically updated to reflect the committed changes.
huntergr marked an inline comment as done.