In D154045#4652092, @aemerson wrote:

We should move this onto GitHub PRs.

Anyway, having a static initializer seems like a bad idea. It seems wasteful to force static initializers to be generated & run just so we can check if we have SME, which in reality is never going to be false if we're in these routines. @rsandifo-arm can we relax this check requirement?

The requirement exists for streaming-compatible calls to normal functions.

LGTM in terms of intent.

LGTM too, thanks.

The intent looks good to me, but I'm not in a position to approve. Very pedantic, sorry, but on the description, I think:

Superceded by D148700

Thanks for dropping the attribute requirements down to TargetAArch64. But the offsetting requirement was that something somewhere (CodeGen?) must raise an error if code requires SME and SME isn't present. I think that means:

Mostly LGTM from a spec POV, but some comments below.

Do you have any more thoughts on the above? Quick version is:

1. Is it OK to have [[…]] attributes in the arm namespace that affect semantics?
2. Is it OK to raise an error for unrecognised attributes in the arm namespace (for a measure of future-proofing)?

In D148700#4401451, @rsmith wrote:

In D148700#4401353, @jyknight wrote:

Yes, standard attributes aren't supposed to be used for things which affect the type system (although, we certainly have many, already, which do, since we expose most GCC-syntax attributes also as C++-standard attribute syntax!)

The rule that standard attributes don't affect semantics only applies to attributes specified by the language standard. There is no expectation that vendor attributes avoid such effects.

Update base commit.

Rebase to account for a diagnostic that has moved.
No other changes from the previous version.

Avoid most uses of << 0. Add comments to those that remain.

Thanks @aaron.ballman and @erichkeane for your patience in reviewing the patch and steering me in the right direction.

Update onto current main. Only change is to replace C++2b with C++23.

In D148700#4364229, @aaron.ballman wrote:

This basically LGTM, but we should add documentation to the internals manual too: https://clang.llvm.org/docs/InternalsManual.html#spellings

Add internals documentation. Add FIXME to temporary code.

Hi @erichkeane and @aaron.ballman. Does the updated patch look OK?

Thanks @erichkeane . Adding the documentation with that kind of disclaimer sounds good to me. I've done that in the updated version, and also fixed the comment problem that Aaron pointed out.

Add documentation. Fix a comment cut-&-pasto that Aaron pointed out.

Hi @aaron.ballman and @erichkeane . Do you have any more thoughts on this? In principle, I'm happy to convert an existing attribute over to the new scheme, but in practice, I can't find one that seems to be suitable. If we're going to do that, I think I'll need guidance as to which attribute to convert.

Thanks for the review.

In D148700#4280833, @erichkeane wrote:

Generally ok with all of this, but want some time to think about it/give aaron/etc time to see it.

Add #ifndef guard.

Make AttrTokenKinds.inc use a new KEYWORD_ATTRIBUTE macro, rather than
using KEYWORD directly. Move the #undef to the .inc file.

Thanks for the reviews!

See https://reviews.llvm.org/D139028 for previous discussion on this topic. https://reviews.llvm.org/D148700 contains the Attr.td support (and the main rationale) while https://reviews.llvm.org/D148702 contains the main Parse & Sema support.

Tweak initializer to avoid an MSVC error: https://godbolt.org/z/sMxsW8G8j

Gah, sorry. Due to a botched git operation, I posted a first cut with a stupid typo, rather than the version that passed testing.

FWIW, the original reason for looking at this was to explore ways of removing AS_AttributeLikeKeyword from https://reviews.llvm.org/D139028 . I wanted to find a way of conveying extra information about a spelling without having to change the existing enums. It seemed like that was essentially the same problem as the one described in the FIXME being fixed here.

I threw together a patch to make the constructors explicit and the only two compile failures I have are with what you're fixing in this patch. If you'd like, I can commandeer this patch and subsume it with the larger refactor. Alternatively, we can land this (I'd drop the test though) and I can rebase on top of your changes. Either is fine by me.

In D147661#4249004, @erichkeane wrote:

This test doesn't show a change in behavior.

Yeah. Is there a way to force the syntax and spelling fields to be dumped, even for implicit attributes? I was struggling to find one, which is why I wasn't sure this made a difference in practice.

In D147661#4248574, @erichkeane wrote:

I think the change is probably fine,

Add AST dump test

Thanks @aaron.ballman and @erichkeane for the comments. I'll split it into stages and use tablegen more, like you say.

I'm just responding to the above without having full context, so sorry if this is rehashing old discussions.

Ping. If this approach is acceptable, I think we could use it for future ACLE features too (rather than the GNU attributes that we've used previously).

Reupload with proper linting. No change in code.

In D127762#4023803, @aaron.ballman wrote:

In D127762#3960906, @rsandifo-arm wrote:

Thanks @aaron.ballman for the feedback about spellings. I've gone with the lower-case names like __arm_streaming in what follows, as a plausible strawman.

My main concern with keywords is:

This approach means there's plenty of flexibility available to parse the keyword where you think it makes sense.

If everyone parses keywords where it makes sense to them personally, for their specific use case, I'm worried that we'll end up with a lot of slightly different rules. (This tended to happen for pre-standard features, and like you say, is still the case for GNU attribute handling between Clang and GCC, given GCC's poorly-defined rules.)

Ah, I meant more that we have flexibility on what we want the grammar of the extension to be. If other implementations wanted to pick up the functionality, hopefully they'd pick up the same grammar as well.

Thanks @aaron.ballman for the feedback about spellings. I've gone with the lower-case names like __arm_streaming in what follows, as a plausible strawman.

Thanks for the patch. This is going to be inconvenient, sorry, but: while implementing the specification in GCC, I noticed that the ZA functions weren't consistent about whether they had an _m suffix. svwrite (MOVA) had one, but the MOP intrinsics that you're implementing here didn't. Since SME2 does have some unpredicated instructions, it seems like it would be better to make the MOP intrinsics consistent with svwrite, with an _m suffix.

I can see at least three ways of handling this:

1. @paulwalker-arm's suggestion to allow -march= without a base architecture (or with a dummy base architecture that means “no change”)
2. a single new option -mfoo= that applies on top of -march and can be used to add or remove architecture features
3. individual -m and -mno- options for each ISA feature.

This approach looks good to me FWIW. The main advantages as I see it are:

• Treating PSTATE.SM and PSTATE.ZA as orthogonal to the set of implemented features follows the scheme used in the ISA specification, so it's easy to cross-check the two. The IsSVEEnabled review comment is an example of this.
• It is likely to cope better with any future extensions to SVE or SME: the streaming-compatible, streaming and non-streaming subsets of each ISA extension would not need to be identified individually on the command line or in IR. Or to put it another way: requiring multiple +streaming-foo flags (and so repeating the “streaming”) feels like specifying the same bit of information multiple times.
• It avoids the risk of combinatorial explosion if we ever need to do something similar for another degree of freedom.

Sorry to ask about an already-closed ticket, but: it looks like this makes the register unconditionally available, rather than making it conditional on FeaturePerfMon. Is that the intended behaviour? GNU as does still require v8.4 for this register.