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

Thu Nov 10 06:43:21 PST 2022

aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.

Adding Erich as the new attributes code owner.

================
Comment at: clang/include/clang/Basic/Attr.td:2322

+def ArmStreamingCompatible : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64> {
+  let Spellings = [Clang<"arm_streaming_compatible">];
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > sdesmalen wrote:
> > > aaron.ballman wrote:
> > > > sdesmalen wrote:
> > > > > aaron.ballman wrote:
> > > > > > `DeclOrTypeAttr` is only intended to be used for attributes which were historically a declaration attribute but are semantically type attributes (like calling conventions). It seems to me that each of these is purely a type attribute and we shouldn't allow them to be written in the declaration position. WDYT?
> > > > > In this case, I believe the attribute is valid on both the type and the declaration, because the SME ACLE explicitly specifies that the attributes can be applied to both declarations and types.
> > > > > 
> > > > > The [[https://github.com/ARM-software/acle/pull/188/commits/ce878cf6c43fd824ffc634526e5dfec1ddc1839e#diff-516526d4a18101dc85300bc2033d0f86dc46c505b7510a7694baabea851aedfaR8283-R8294|specification]] says:
> > > > > ```Some of the attributes described in this section apply to function types.
> > > > > Their semantics are as follows:
> > > > > 
> > > > > *   If the attribute is attached to a function declaration or definition,
> > > > >     it applies to the type of the function.
> > > > > 
> > > > > *   If the attribute is attached to a function type, it applies to that type.
> > > > > 
> > > > > *   If the attribute is attached to a pointer to a function type, it applies
> > > > >     to that function type.
> > > > > 
> > > > > *   Otherwise, the attribute is [ill-formed](#ill-formed).```
> > > > > In this case, I believe the attribute is valid on both the type and the declaration, because the SME ACLE explicitly specifies that the attributes can be applied to both declarations and types.
> > > > 
> > > > What are the chances that can change? Because there are problems here:
> > > > 
> > > > > If the attribute is attached to a function declaration or definition, it applies to the type of the function.
> > > > 
> > > > This is egregiously opposed to the design of `[[]]` attributes in both C and C++. We came up with `DeclOrTypeAttr` for attributes that previously existed, but that is different than introducing new attributes. It's par for the course for `__attribute__` style attributes, so I don't worry so much there.
> > > > 
> > > > What's the rationale for this confusing of a design? (e.g., is there some reason you basically have to do that, like historically accepting the attributes in both positions?)
> > > The attribute must always apply to the type of the function, because we can't have the streaming-property (or the properties on ZA) being lost between function overrides or function pointer assignments. It's perhaps similar to a calling convention, because the caller may have to set up streaming- or ZA state before the call, and restore state after the call.
> > > 
> > > I'm not too familiar with the different spellings/syntaxes and their implications, so I've now limited the attribute to `GNU` style attributes (`__attribute__((..))`) and to being a `TypeAttr`, with the exception of the `arm_locally_streaming` attribute, because that one can only be applied to function definitions and is not part of the type.
> > > 
> > > I've also added some new tests to ensure the properties are correctly propagated (using `decltype()`) and tests to ensure virtual function overloads require the same attributes.
> > > 
> > > Is this a step in the right direction? :)
> > Switching to a GNU-only spelling sort of helps, but I still think this design is confused.
> > ```
> > *  If the attribute is attached to a function declaration or definition,
> >     it applies to the type of the function.
> > ```
> > This seems like a misdesigned feature and I think it should be fixed in the specification. If it's an attribute that applies to the type of the function, you should not be allowed to specify it on the declaration; what is the benefit to allowing it in an unexpected position?
> > If it's an attribute that applies to the type of the function, you should not be allowed to specify it on the declaration
> Could you maybe give me an example here of what it looks like to apply the attribute to the function *type* when it's not allowed on the *declaration*? Does this all have to do with the //position// of the attribute in the declaration statement? When writing a function declaration it defines both the name and the type of that function, so there's no way to write a declaration without also defining its type.
> 
> Maybe it helps if you have any pointers that explain the difference in syntax between function declaration/function type attributes (https://clang.llvm.org/docs/AttributeReference.html doesn't really address this)
> Could you maybe give me an example here of what it looks like to apply the attribute to the function *type* when it's not allowed on the *declaration*? Does this all have to do with the position of the attribute in the declaration statement? When writing a function declaration it defines both the name and the type of that function, so there's no way to write a declaration without also defining its type.

Sure, but I'm going to use `[[]]` syntax to demonstrate the difference because it's very hard to reason about *what* a GNU-style attribute applies to given it's "sliding" behavior. C++ attributes have rules about what they appertain to based on syntactic location of the attribute so it's an easier way to demonstrate.

https://godbolt.org/z/Yz37fTbWY demonstrates some of the differences in terms of positioning of the attribute.
https://godbolt.org/z/18Y1Pn4se demonstrates the behavior when the type attribute matters for the type identify of the function.
https://godbolt.org/z/1jTqPx4f4 demonstrates another way in which a type attribute can matter.

For `__attribute__` it's harder to reason about because of https://gcc.gnu.org/onlinedocs/gcc/extensions-to-the-c-language-family/attribute-syntax.html, especially because GCC and Clang don't always agree in terms of whether something can be used as a type attribute or not. But in terms of the mental model of attributes, the question really boils down to how much the attribute matters to the identity of the type -- if you don't want users to be able to assign an attributed function to a nonattributed function pointer (or vice versa), it most likely is a type attribute. If you think presence or absence of the attribute should impact things like overload resolution or template specializations, it's almost certainly a type attribute.

================
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:
> 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?"

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762