[PATCH] D127762: [Clang][AArch64] Add ACLE attributes for SME.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Type.h:4008
     bool HasTrailingReturn : 1;
+    unsigned AArch64SMEAttributes : 8;
     Qualifiers TypeQuals;
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > sdesmalen wrote:
> > > > aaron.ballman wrote:
> > > > > So functions without prototypes cannot have any of these attributes?
> > > > Yes, the ACLE explicitly states that [[https://github.com/ARM-software/acle/pull/188/commits/59751df91d9630400531a64108f179e3951c3b89#diff-516526d4a18101dc85300bc2033d0f86dc46c505b7510a7694baabea851aedfaR503|here]]:
> > > > > The function type attributes cannot be used with K&R-style “unprototyped” C function types
> > > Are they aware that includes; `void Baz();` ?
> > FWIW, the linked docs say:
> > ```
> > > Functions like `some_func` and `another_func` are referred to as
> > > (K&R-style) “unprototyped” functions. The first C standard categorized
> > > them as an obsolescent feature and C18 removed all remaining support
> > > for them.
> > ```
> > That's not quite accurate. They've always been obsolete in standard C (both ANSI and ISO C), so that's correct. But it's C2x that removes all remaining support for them. (Also, there's no such release as C18, there's C11, C17, and expected to be C23).
> > 
> > > Are they aware that includes; void Baz(); ?
> > 
> > Just to expound on this question: this means `void baz();` in C17 mode cannot have the attribute but `void baz();` in C2x mode can. Folks coming from C++ tend to expect the C2x behavior to be the default and so it's plausible to see such signatures in older language modes. Will that cause any pain or confusion for you?
> @rsandifo-arm explained to me that it is indeed a deliberate choice not to support the attribute on unprototyped functions, as these are an obsolescent feature that will be removed in C2x. The mitigation should be trivial for new code that is written and it makes new code compliant with the next version of the standard.
Perfect, thank you for double-checking!


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6322
+
+Only when Clang defines __ARM_FEATURE_SME, the support for this feature is
+considered to be stable and complete.
----------------
Double-checking: that macro is missing the trailing double underscore, is that expected?


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2000
   "overridden virtual function is here">;
+def err_conflicting_overriding_attributes : Error<
+  "virtual function %0 has different attributes "
----------------
rsandifo-arm wrote:
> aaron.ballman wrote:
> > rsandifo-arm wrote:
> > > rsandifo-arm wrote:
> > > > aaron.ballman wrote:
> > > > > aaron.ballman wrote:
> > > > > > rsandifo-arm wrote:
> > > > > > > sdesmalen wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > sdesmalen wrote:
> > > > > > > > > > aaron.ballman wrote:
> > > > > > > > > > > This error and the one below make me wonder whether an attribute spelling is the appropriate way to surface this functionality as opposed to a keyword. Attributes don't typically don't cause errors in these situations, but because these attributes are strongly coupled to their type system effects I can see why you want these to be errors.
> > > > > > > > > > > This error and the one below make me wonder whether an attribute spelling is the appropriate way to surface this functionality as opposed to a keyword. 
> > > > > > > > > > I'm not sure I understand what you mean, do you have an example?
> > > > > > > > > `override` and `final` could have been surfaced via attributes, and in Clang we semantically express them as such (see `Final` and `Override` in Attr.td), but instead they are surfaced to the user as keywords in the language standard. You're not under the same constraints as the standard (you're making a vendor-specific attribute). We're not super consistent with whether we use the same approach as the standard (we have some type attributes that are spelled as attributes like `vector_size` and others that are spelled via keywords like `_Nullable`.
> > > > > > > > > 
> > > > > > > > > So you could spell your type attributes the way you have been with `__attribute__`, or you could come up with keywords for them (so instead of using `GNU<"whatever">` for the attribute, you could use `Keyword<_Whatever>` or `Keyword<__whatever>` (you'd also need to add them as recognized keyword tokens, parse them as appropriate, etc).
> > > > > > > > > 
> > > > > > > > > Note: I don't insist on you using a keyword for these, but I am wondering if that approach was considered or not given how non-portable the attributes are (if an implementation ignores this attribute, it sounds like the program semantics are unlikely to be correct).
> > > > > > > > @rsandifo-arm can you shed some light on whether using a keyword instead of an `attribute` was considered?
> > > > > > > Thanks @aaron.ballman for the reviews.
> > > > > > >  
> > > > > > > Yeah, we did consider using keywords instead.  The main reason for sticking with GNU attributes is that they were specifically designed as an extensible way of attaching information or requirements to the source code, and they provide well-settled semantics.  It seemed better to reuse an existing mechanism rather than invent something new.
> > > > > > > 
> > > > > > > Also, as C++ evolves, the semantics of GNU attributes evolve to match.  If we surface the SME features as GNU attributes, we inherit this development in the underlying semantics, without having to maintain our own evolving specification for where the keywords can be placed.  For example, when lambdas were introduced, GNU attributes were extended to support lambdas.  Something similar could happen in future.
> > > > > > > 
> > > > > > > We could have defined the semantics of the keywords to be that they behave exactly like GNU attributes.  However:
> > > > > > > 
> > > > > > > # If we do that, the advantage of using a keyword is less obvious.  (I can see the argument that the syntax looks nicer though.)
> > > > > > > # Like you say in the review, the semantics of GNU attributes carry some historical baggage.  If would be difficult to justify keeping that historical baggage for something that is ostensibly new and not a GNU attribute as such.
> > > > > > > 
> > > > > > > A minor, but still nontrivial, reason is that there is less appetite in the GCC community for supporting target-specific extensions to the core language.  Adding a target-specific keyword is likely to be rejected.  It would be acceptable to make the “keyword” be a predefined macro that expands to a GNU attribute under the hood, but I don't think that would address the core issue.
> > > > > > > 
> > > > > > > I can definitely understand the unease about using attributes for things that affect semantics.  Like you say, that's going against a core principle of the standard-defined attributes.  But I think in a way, it's unfortunate that GNU-style attributes and standard-defined attributes are both called “attributes”, because I don't think there's a prohibition (even in theory) against GNU attributes affecting semantics.  I think GNU attributes are designed to be more general than that, probably through historical accretion rather than an up-front choice.
> > > > > > > 
> > > > > > > Like you say, `vector_size` is one example of something that significantly affect semantics.  But there are quite a few others too.  For example:
> > > > > > > * GNU targets traditionally provide access to naked functions and interrupt handlers through attributes.
> > > > > > > * Different calling conventions also use attributes.
> > > > > > > * The `target` attribute switches between ISAs in ways that do affect semantics.
> > > > > > > * `always_inline` functions must be inlined for correctness.
> > > > > > > * `weak` and `visibility` in effect alter the ODR.
> > > > > > > * In GCC, functions that return twice (like `setjmp`) must be declared `returns_twice`.
> > > > > > > 
> > > > > > > It's unfortunate that using SME attributes will only trigger a warning in older compilers.  The warning is enabled by default though.  And in practice, I think most SME code would rely on other constructs that older compilers wouldn't provide, such as intrinsics.
> > > > > > > Yeah, we did consider using keywords instead. The main reason for sticking with GNU attributes is that they were specifically designed as an extensible way of attaching information or requirements to the source code, and they provide well-settled semantics. It seemed better to reuse an existing mechanism rather than invent something new.
> > > > > > 
> > > > > > If this is extra information the compiler is free to ignore without impacting program correctness, then I think an attribute is fine. But if the user's code will silently break if the attribute is ignored (e.g., an attribute ignored warning happens but the program successfully translates and does bad things at runtime), in some ways use of an attribute is far less ideal than use of a keyword-based attribute specifically because of the syntax error the user would get. Some users really like the "unknown attribute ignored" warning (like me!) and ensure it stays enabled and others users really don't like that warning and disable it. So it's dangerous to assume there's anything there worth relying on if the attributes have security implications at runtime.
> > > > > > 
> > > > > > > Also, as C++ evolves, the semantics of GNU attributes evolve to match. 
> > > > > > 
> > > > > > Errr does it? https://gcc.gnu.org/onlinedocs/gcc/extensions-to-the-c-language-family/attribute-syntax.html#attribute-syntax "There are some problems with the semantics of attributes in C++. For example, there are no manglings for attributes, although they may affect code generation, so problems may arise when attributed types are used in conjunction with templates or overloading. Similarly, typeid does not distinguish between types with different attributes. Support for attributes in C++ may be restricted in future to attributes on declarations only, but not on nested declarators." doesn't really seem to support that position.
> > > > > > 
> > > > > > > A minor, but still nontrivial, reason is that there is less appetite in the GCC community for supporting target-specific extensions to the core language. 
> > > > > > 
> > > > > > That's unfortunate given that `_Keywords` are reserved to the implementation specifically for this sort of thing. `_Nullable` is a good example of a successful attribute keyword. Attributes are a target-specific extension to the core language regardless of what syntax they use, so to me the driving question is more "what user experience do you want?" and less "is this an extension to the core language?"
> > > > > Re-pinging this part of the thread -- the more I review this, the more it sounds like ignoring these attributes will cause a significant likelihood of the user's program silently misbehaving. Is that accurate? If so, I would reconsider whether using a keyword gives a more appropriate user experience for portability. With an attribute, the user has a very real chance of their code continuing to compile but not work. With a keyword, the user will get errors when porting to a compiler that doesn't support the keyword and are more protected from miscompiles. We can still reuse the vast majority of the implementation work already done here, so it should not be a major change to the implementation (a bit of extra parsing work for the keywords is really the only thing missing).
> > > > I accept your points about the dangers of ignoring unsupported attributes.  But this isn't an SME-specific issue.  Ignoring the other GNU attributes I mentioned would also silently generate wrong code.  I think this is a very strong argument against suppressing warnings about unrecognised GNU attributes by default, even if warnings about unrecognised standard attributes are eventually suppressed by default.  (Perhaps they should even be controllable separately?)  Like I say, there doesn't seem to have been a principle that GNU attributes provide information that the compiler is free to ignore.
> > > > 
> > > > If we did add keywords, would you be OK with giving them exactly the same semantics as GNU attributes?  In other words, would it be acceptable to treat the keywords as essentially a one-for-one parsing replacement for GNU attributes?  Or would you want the keyword to follow the same distinction between type properties and object properties as the standard attributes do?
> > > > Or would you want the keyword to follow the same distinction between type properties and object properties as the standard attributes do?
> > > That was a vague question, sorry.  What I meant is whether the correct position of the keyword should vary based on whether it describes a property of the function type vs. a property of the function definition, or whether we can maintain the laxity in the GNU attribute positioning.  I think the laxity in the GNU attribute positioning is helpful to users, and in this case wouldn't introduce any ambiguity.
> > > 
> > > From discussions I've had with people who will write/are writing SME code, the distinction between `arm_locally_streamng` being a property of the definition and `arm_streaming` being a property of the type seems contrived to them, especially when the usage of SME is private to a single translation unit.
> > > 
> > > Treating the keyword as an error-generating GNU attribute stand-in would also allow implementations to make the keyword as a preprocessor macro, if they need to.
> > > If we did add keywords, would you be OK with giving them exactly the same semantics as GNU attributes?
> > 
> > The way we do keyword attributes is that the parser is responsible for parsing the keyword and kicking off the creation of the attribute in the proper place. For example: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L928. This approach means there's plenty of flexibility available to parse the keyword where you think it makes sense.
> > 
> > That said, function types don't always allow for qualifiers (for example, free functions in C++ and all functions in C), so I would expect something more like `_Keyword int func();` for functions and probably like `int (* _Keyword)();` for function pointers. However, I agree that it's a bit more awkward for SME needs than the nullability attributes because the nullability attributes map directly onto qualifiers and so the question of 'where do we parse this' was very easy to answer.
> > 
> > All this said, if there's opposition to using keywords and a preference for attributes, that is okay by me (we have other type attributes). I was bringing it up mostly to make sure the design was considered explicitly rather than thinking an attribute was the only viable approach.
> > The way we do keyword attributes is that the parser is responsible for parsing the keyword and kicking off the creation of the attribute in the proper place. For example: https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L928. This approach means there's plenty of flexibility available to parse the keyword where you think it makes sense.
> > 
> > That said, function types don't always allow for qualifiers (for example, free functions in C++ and all functions in C), so I would expect something more like `_Keyword int func();` for functions and probably like `int (* _Keyword)();` for function pointers.
> 
> Thanks.  Having a consistent position like `_Keyword int func();` for both kinds of attribute (decl and type) sounds good.
> 
> If we did add keywords, what kinds of spelling would you suggest?  In the GNU attribute names we used the `arm_` prefix as a kind of old-school namespace emulation.  If the information had been suitable for standard attributes, we would obviously have used `arm::` instead.  However, using namespace-like prefixes might not be as natural for keywords, and I couldn't see any existing examples in `TokenKinds.def`.  Would names like `_NewZA`/`__new_za` be OK?  Would you recommend the CamelCase or lower_case style?
> 
> If we did use unprefixed keywords, would `_Streaming`/`__streaming` be acceptable for streaming functions?  Or, since the term “streaming” is pretty generic, should we try to make the spelling more specific to AArch64 or SME?
> 
> Also, in terms of Clang direction, would it be acceptable to continue to add target-specific keywords for each new feature that compilers must interpret for correctness, and handle them directly in things like the parser case statements?  Or should there be a more generic way of handling this, similar to `Attr.td`?  The reason for asking is that it seems like the list of keywords could become quite long, just like `Attr.td` has.  I don't think the SME attributes/information are particularly unusual, and it seems a lot of existing attributes would have been handled this way if we were starting from scratch, including all those that affect calling conventions.  (There are some calling conventions that already have keywords, but there are some like `preserve_most` and `preserve_all` that seem to be handled by attributes only.  There's also the Arm-specific `aarch64_vector_pcs`.)
> If we did add keywords, what kinds of spelling would you suggest?

Ooof, yeah, you're asking some good questions about names! I'm hesitant to take names that are generic because of the potential for conflicts with other targets. However, for concepts like streaming, using `_Streaming` as the keyword is rather appealing too. The one scenario that really makes me think we should *not* use generic keywords is if the user tries to combine extensions. e.g., if ARM SVE uses `_Streaming` for one purpose and SYCL uses `_Streaming` for a slightly different purpose, it becomes impossible to use SYCL and ARM SVE together in some circumstances because we might not know which semantics to pick for any given use of the keyword. That makes me think `_Arm_Streaming` or `__arm_streaming` or whatever is a better approach. But as you point out, our existing keyword attributes don't encode a prefix in their names so this is novel. In terms of camel case vs lower case, I don't have a strong opinion on which to go with; we've used both in the past (`__global` and `__constant` vs `_Nullable` and, but `__lower_case` is by far the most prevalent spelling variety.

> Also, in terms of Clang direction, would it be acceptable to continue to add target-specific keywords for each new feature that compilers must interpret for correctness, and handle them directly in things like the parser case statements? Or should there be a more generic way of handling this, similar to Attr.td?

I think we can probably tablegen more than we do already (for example, it would be nice for TokenKinds.def and Attr.td to be hooked together so you don't need to specify the spelling of the keyword in two places), but I'm not confident we can make it as general as attributes given the parsing implications. The closest idea I had would be to encode some kind of "this is a decl specifier" vs "this is a type specifier" vs "this is written on the declarator" kind of information in Attr.td, have some generic code in the parser that says "now parse keyword attributes in <whatever syntactic location>" that dispatches to some tablegenerated code which eats the correct tokens. However, I would expect this to be somewhat inflexible for parsing (e.g., ordering between keywords and other syntax) and maybe not suitable for all keyword attributes (parameterized keywords, for example). But if we started with the use cases we have, maybe it would generalize well enough to be worth it?

For reference, in terms of kinds of existing keywords in Attr.td, we have:

Decl: 8
Stmt: 0
Type: 16
Decl Or Stmt: 2
Decl Or Type: 8
Ignored: 1

So there's a reasonable variety of cases to have to think about for a generalized solution, but we could perhaps tackle them one at a time (start with type attributes because they're so popular and then add declaration attributes later and have more parsing locations).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762