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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

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

> Gentle ping @aaron.ballman and @erichkeane. I've addressed all comments and suggestions and believe we found agreement on the approach (to use a keyword instead of an attribute).
> Are you happy to accept the patch?

I think things are looking close, still some minor issues to correct though.



================
Comment at: clang/include/clang/AST/Type.h:3987
     /// [implimits] 8 bits would be enough here.
-    uint16_t NumExceptionType = 0;
+    uint16_t NumExceptionType;
+
----------------
I think this should also be an `unsigned` bit-field so that it packs together with the new field added below. Otherwise we're liable to end up with padding between this field and the next bit-field (which will take a full allocation unit anyway). How about `unsigned NumExceptionType : 10;`? We're reducing the count by 6 bits, but.... I struggle to imagine code being impacted and we're still allowing more than the implementation limits require.


================
Comment at: clang/include/clang/AST/Type.h:4172-4174
     bool Variadic : 1;
     bool HasTrailingReturn : 1;
+    unsigned AArch64SMEAttributes : 6;
----------------
IIRC, at least one of the compilers used to build Clang has issues with adjacent bit-fields of differing types, so we'd usually make these `bool` fields be `unsigned` at this point to ensure they pack together properly.


================
Comment at: clang/include/clang/AST/Type.h:4204-4205
+      else
+        AArch64SMEAttributes =
+            (AArch64SMEAttributes ^ Kind) & AArch64SMEAttributes;
     }
----------------
Isn't the more idiomatic form of this written as `AArch64SMEAttributes &= ~Kind;`?


================
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).
----------------
rsandifo-arm wrote:
> How about “non-streaming” rather than “normal”?
non-streaming sounds good to me.


================
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<
----------------
rsandifo-arm wrote:
> 
Also, remove the single quotes around %0 and %1 -- you're getting duplicate quotes in the test cases because of them.


================
Comment at: clang/lib/Sema/SemaType.cpp:7937
+
+    const FunctionProtoType *FnTy = unwrapped.get()->getAs<FunctionProtoType>();
+    if (!FnTy) {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762