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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D127762#3615702 <https://reviews.llvm.org/D127762#3615702>, @sdesmalen wrote:

> Thanks for your patience reviewing this patch @aaron.ballman!

Happy to help, thank you for your patience with my constant stream of questions about the design. :-)



================
Comment at: clang/include/clang/AST/Type.h:4064
     bool HasTrailingReturn : 1;
+    unsigned AArch64SMEAttributes : 8;
     Qualifiers TypeQuals;
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > If we're taking up space in the extra bitfields, why do we need to also take up space in the function prototype itself?
> This field is added to ExtProtoInfo struct, which is used to populate the ExtraBitfields in the `FunctionPrototype`'s AST node.
> 
> Sorry if this wasn't clear because the diff had no context. `arc diff` wasn't working for me, so manually updated the patch, but then forgot to add full context.
Ah, I see what's going on there, thank you (the extra context helped as well).


================
Comment at: clang/include/clang/Basic/Attr.td:2333
 
+def ArmStreamingCompatible : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
+  let Spellings = [GNU<"arm_streaming_compatible">];
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > You are handling these as declaration attributes in SemaDeclAttr.cpp but declaring them to be type attributes here in Attr.td; that's not okay. If these are type attributes, there should not be code for them in SemaDeclAttr.cpp. (This is why I really think the design needs to be rethought; I think they should be type attributes only, but I think you're trying to match what your specification says.)
> Okay, I've removed the handling in SemaDeclAttr, so that it is only handled in SemaType.cpp.
> 
> But it still allows the attribute to be applied to any position in the function declaration, i.e.
> 
>   __attribute__((arm_streaming)) float foo1(int);
>   float __attribute__((arm_streaming)) foo2(int);
>   float foo3(int) __attribute__((arm_streaming));
> 
> Is that expected?
https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax

"For compatibility with existing code written for compiler versions that did not implement attributes on nested declarators, some laxity is allowed in the placing of attributes. If an attribute that only applies to types is applied to a declaration, it is treated as applying to the type of that declaration. If an attribute that only applies to declarations is applied to the type of a declaration, it is treated as applying to that declaration; and, for compatibility with code placing the attributes immediately before the identifier declared, such an attribute applied to a function return type is treated as applying to the function type, and such an attribute applied to an array element type is treated as applying to the array type. If an attribute that only applies to function types is applied to a pointer-to-function type, it is treated as applying to the pointer target type; if such an attribute is applied to a function return type that is not a pointer-to-function type, it is treated as applying to the function type."

So yes, that's expected because the attribute only applies to types, so it magically "slides" onto the type regardless of where it was written on the declaration. By making it a type only attribute, it's now clear what the GNU attribute applies to regardless of where it's written.


================
Comment at: clang/include/clang/Basic/Attr.td:2345
+
+def ArmLocallyStreaming : DeclOrStmtAttr, TargetSpecificAttr<TargetAArch64SME> {
+  let Spellings = [GNU<"arm_locally_streaming">];
----------------
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`.


================
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 "
----------------
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).


================
Comment at: clang/lib/Sema/SemaType.cpp:7669-7672
+    const FunctionProtoType *FnTy = unwrapped.get()->getAs<FunctionProtoType>();
+    if (!FnTy) {
+      // SME ACLE attributes are not supported on K&R-style unprototyped C
+      // functions.
----------------
sdesmalen wrote:
> aaron.ballman wrote:
> > Ah, I think the `Subjects` list in Attr.td is incorrect then; it seems like you want to use `HasFunctionProto` instead of `FunctionLike` (or something more similar to `HasFunctionProto` if you really don't want to support ObjC methods or blocks).
> Even when I set that to `HasFunctionProto`, this check still seems necessary. When I remove it, one of the tests fails because `FnTy` is `nullptr`). Is that to be expected?
I'm sorry, I see now that I forgot to say the important bit that was in my head: we don't check those subjects automatically yet, but we do want to add more automation to type attribute processing as we've already done for declarations and statements, so we want the content in Attr.td to be as correct as possible even though it still needs to be manually checked. That makes it easier on us when we go to add the automation; there's less "which is correct, the SemaType.cpp code or the Attr.td declaration?" involved).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762