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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a subscriber: rsandifo-arm.
sdesmalen added a comment.

The past few months we've worked to get the attributes at the LLVM IR side implemented. Since that work has now landed, this patch should no longer be held up by the LLVM side of things.

@aaron.ballman I've updated and rebased the patch. I went through all the comments again and believe that all of them have been addressed. Would you be happy to give this patch another look?



================
Comment at: clang/include/clang/Basic/Attr.td:2345
+
+def ArmLocallyStreaming : DeclOrStmtAttr, TargetSpecificAttr<TargetAArch64SME> {
+  let Spellings = [GNU<"arm_locally_streaming">];
----------------
aaron.ballman wrote:
> sdesmalen wrote:
> > aaron.ballman wrote:
> > > Why is this a *statement* attribute?
> > You're right that it shouldn't be. I want it to just be a decl attribute and don't want it to be either a Type or a Stmt attribute, but that doesn't seem to exist.
> > I find their meaning quite confusing, because both allow me to limit their applicability to Functions. What is the right class to derive from here?
> Hmm, we have some documentation at https://clang.llvm.org/docs/InternalsManual.html#how-to-add-an-attribute but it's not very clear on all the choices and when to pick which one.
> 
> For declaration attributes on a function, the question really boils down to whether you want redeclarations to automatically inherit the attribute or not. e.g.,
> ```
> __attribute__((arm_locally_streaming)) void func(void);
> void func(void); // Is this declaration also marked locally streaming?
> 
> void use(void) {
>   func(); // Does this use see the attribute or not?
> }
> ```
> If you want the call in `use()` to see the attribute, then you want the attribute to be inherited on the redeclaration, and so you'd use `InheritableAttr`. If you don't want the call in `use()` to see the attribute, then you'd use `Attr`.
Thanks for clarifying. I've changed the code to use `InheritableAttr`, since future redeclarations/redefinitions should inherit the `arm_locally_streaming` attribute (and also added an extra test-case for this)


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762