[PATCH] D127762: [Clang][AArch64] Add/implement ACLE keywords for SME.

[Richard Sandiford via Phabricator via cfe-commits]

rsandifo-arm added a comment.

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



================
Comment at: clang/include/clang/Basic/Attr.td:2439
 
 def ArmStreaming : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
   let Spellings = [RegularKeyword<"__arm_streaming">];
----------------
In current main, this attribute is gated on TargetAArch64 rather than TargetAArch64SME.  The reason is that, from a QoI point of view, I don't think we should require the `+sme` extension in order to accept an `__arm_streaming` type.  `+sme` is instead required to compile a function with `__arm_streaming` type, or a call to a function with `__arm_streaming` type.  IMO those are more CodeGen rather than Sema restrictions.

E.g., if a header file contains
```
void sme_foo(void) __arm_streaming;
```
then an `__attribute__((target_version("sme")))` function should be able to call it.  But we shouldn't require `+sme` to be passed on the command line in order to compile the declaration of `sme_foo`.

I think the same principle applies to the other function-type attributes.  In particular, `__arm_preserves_za` functions and `__arm_streaming_compatible` functions do not require SME to be present.

I think we should also allow:
```
__arm_locally_streaming void __attribute__((target_version("sme"))) ifunc() { … }
```
since `__arm_locally_streaming` does not change the function signature.

So I think all of these attributes should be gated on TargetAArch64, with checks for SME made elsewhere where necessary.

(Unfortunately, since command-line options are out of scope for ACLE, I don't think it can really specify this.)


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6588
 
-* the function requires the Scalable Matrix Extension (SME)
+* the function requires that the target processor implements the Scalable Matrix
+  Extension (SME).
----------------
The bullet was trying to describe the function's ABI.  I don't think “target” is appropriate in an ABI context (where you might be describing a binary that already exists, rather than a compilation).  So I've a slight preference for dropping “target”.

I see in another comment, Erich made a distinction between “target” and “run-time”.  If we do use one or the other, I think “run-time” is more correct.

“target” is right for `__arm_locally_streaming` and `__arm_new_za` though.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6623
+
+* the function may be entered in either normal mode (PSTATE.SM=0) or
+  in streaming mode (PSTATE.SM=1).
----------------
How about “non-streaming” rather than “normal”?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6626
+
+* the function must return in the same mode in which it entered the function.
+
----------------



================
Comment at: clang/include/clang/Basic/AttrDocs.td:6666
+
+* the function returns with ZA in an active state for normal returns.
+
----------------
The other type attributes also only describe normal returns.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6738
+
+* the function will commit any lazy-saves if available.
+
----------------
Just trying to avoid “if available”, since the saves aren't being provided for the benefit of anyone else.  Feel free to reword to something else.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3608-3609
+def err_sme_attr_mismatch : Error<
+  "function declared '%0' was previously declared '%1'"
+  " with different SME function attributes">;
 def err_cconv_change : Error<
----------------



================
Comment at: clang/lib/AST/Type.cpp:3559
   //      bool*
   // Finally, we have the ext info and trailing return type flag:
   //      int bool
----------------
This comment probably needs updating for the new field, although the comment already seems pretty out of date.  (It looks like the separate bool was removed in 2011.)

I suppose the new field doesn't create any ambiguities because it's always present.  But given the comment about performance sensitivity, I wonder if we should combine the new field with the trailing return flag?


================
Comment at: clang/lib/AST/TypePrinter.cpp:943
+       FunctionType::SME_PStateSMCompatibleMask) &&
+      !InsideCCAttribute)
+    OS << " __arm_streaming_compatible";
----------------
Are the `!InsideCCAttribute` conditions necessary?  AIUI, InsideCCAttribute exists to suppress the default calling convention when printing the type to which an explicit CC is added.  That doesn't apply to SME attributes, because they aren't modelled as traditional CCs, and because the default/normal case doesn't have any syntax associated with it.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2291
 
+  // We only need to handle the 'arm_locally_streaming' attribute as a
+  // special case here (as opposed to e.g. 'arm_streaming'), because it
----------------
How about something like:
```
// Handle SME attributes that apply to function definitions,
// rather than to function prototypes.
```
I can see why the current comment helped when we were still using GNU attributes, due to the way that type attributes “slid” from the declaration to the type.  But now that the attributes are non-sliding type attributes, it wouldn't make sense to query `D->hasAttr` for them.

The suggested change also avoids the contradiction between “we only need to” and “the same holds for”.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9688
+      Changed == FunctionType::SME_PStateZAPreservedMask &&
+      (ToAttributes & FunctionType::SME_PStateZAPreservedMask);
+  return !DropsPreservesZA;
----------------
The meaning of “from” and “to” seem to be the opposite of what I'd initially assumed.  For casts, I'd have expected the type of the object that is being casted to have FromType and the cast type (the type of the result) to be ToType.  Dropping ZA would then mean that FromAttributes has `__arm_preserves_za` and ToType doesn't.

I think that also makes sense for the virtual function check above.  The overriding function has FromType and the function it overrides has ToType.  We need to be able to convert from FromType to ToType (e.g. to initialise the vtable), but don't need to be able to go the other way.


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:57
+
+// FIXME: Add invalid function pointer assignments such as assigning:
+//   1. A streaming compatible function to a normal function pointer,
----------------
Is it not possible to address this FIXME yet?  Might be worth expanding the comment to say why if so.


================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:184
+
+struct S2 : public S {
+// expected-cpp-error at +2 {{virtual function 'shared_za_memberfn' has different attributes ('void ()') than the function it overrides (which has 'void () __arm_shared_za')}}
----------------
I think it'd be worth having a few more of these, in particular testing that an override can be `__arm_shared_za __arm_preserves_za` if it wants to be.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762