This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][SME] Replace +streaming-sve feature with +streaming-compatible-<feature>.
AbandonedPublic

Authored by sdesmalen on Feb 21 2022, 9:01 AM.

Details

Summary

The current implementation of '+streaming-sve' only supports the SVE(2) and NEON
instructions that are valid in SME's Streaming SVE mode (PSTATE.SM == 1). The
architecture's definition of PSTATE.SM == 1 also includes SME instructions.

Because the SVE(2)/NEON instructions are compatible with both PSTATE.SM ==
0 and PSTATE.SM == 1, the better name for these features is 'streaming
compatible'.

Diff Detail

Event Timeline

sdesmalen created this revision.Feb 21 2022, 9:01 AM
sdesmalen requested review of this revision.Feb 21 2022, 9:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2022, 9:01 AM
paulwalker-arm added inline comments.Feb 22 2022, 5:02 AM
llvm/lib/Target/AArch64/AArch64.td
441–460

Do we need to split the feature flags based on other feature flag boundaries? Why not just FeatureStreamingCompatible? I'm not saying the above is wrong, but it does seem like an extra level of indirection so just trying to understand the reasoning.

llvm/lib/Target/AArch64/AArch64Subtarget.h
136

Up to you but stylistically I think it would be better if all the SME related flags live together. I understand this flag relates to SVE but if it wasn't for SME it wouldn't exist.

c-rhodes added inline comments.Feb 22 2022, 12:29 PM
llvm/lib/Target/AArch64/AArch64.td
441–460

Do we need to split the feature flags based on other feature flag boundaries? Why not just FeatureStreamingCompatible? I'm not saying the above is wrong, but it does seem like an extra level of indirection so just trying to understand the reasoning.

I agree, naming aside I don't understand what the limitations of the existing feature flag are and why this extra granularity at the flag level is required.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
60–69

why is this removed?

sdesmalen added inline comments.Feb 24 2022, 5:32 AM
llvm/lib/Target/AArch64/AArch64.td
441–460

I don't think there is a reason that a single feature wouldn't work. My aim was mostly to remove the 'Streaming SVE' and 'SVE' from the name because 'Streaming SVE' has a special meaning in SME and the term 'SVE' in the name may incorrectly give the impression this is about SVE(1). And hence I started breaking this down into separate features to avoid the SVE suffix. But removing any suffix works as well I guess.

I do expect that we'll have more instructions being added to the set of 'Streaming compatible' instructions over time. I wasn't sure if in C++ code it's easier to reason about "has featureABC or strict subset of featureABC" than it would to reason about "has featureABC or featureXYZ". Anyway, I'm happy to change it if you think that's an improvement. I guess it can always be split up in the future when there's a need for it.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCTargetDesc.cpp
60–69

Given that the streaming mode feature is something internal to the compiler (and not a feature flag that can be set through -march, because only +sme, +sme-i64 and +sme-f64 are user facing), we can have front-end toggle these features rather than doing it implicitly here when creating the subtarget. Through the ACLE it will be possible to define a function as 'streaming compatible' using an attribute. Clang could then set the appropriate -neon,+streaming-compatible-sve feature flags for the IR function.

paulwalker-arm added inline comments.Feb 24 2022, 5:43 AM
llvm/lib/Target/AArch64/AArch64.td
441–460

Thanks @sdesmalen. I would expect future additions to form part of a new spec so it would be treated much like any other extension in that we'd have FeatureStreamingCompatibleVx.y to match a specific level of the architecture/extension.

sdesmalen abandoned this revision.Feb 28 2022, 7:48 AM