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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

I think we need a few tests to ensure that these are properly propagated on function templates (both for Sema and codegen).  Also, a handful of nits (See my list).



================
Comment at: clang/include/clang/AST/Type.h:3958
+    SME_PStateZAPreservedMask = 1 << 3,
+    SME_AttributeMask = 63 // We only support maximum 6 bits because of the
+                           // bitmask in FunctionTypeExtraBitfields.
----------------



================
Comment at: clang/include/clang/Basic/Attr.td:2459
+def ArmPreservesZA : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
+  let Spellings = [RegularKeyword<"__arm_preserves_za">];
+  let Subjects = SubjectList<[HasFunctionProto], ErrorDiag>;
----------------
So, why are `__arm_shared_za` and `__arm_preserves_za` different tenses, despite being very similar sounding?  Should it be `__arm_shares_za` (or alternatively, `__arm_preserved_za`)?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6595
+
+* the function requires the Scalable Matrix Extension (SME)
+
----------------
I'm missing a touch of context that an additional word or two might help.  is this 'the function requires the 'target' processor has SME?  or the 'runtime' processor?

Also, the rest of the list ends in a '.', but this one doesnt.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6633
+It applies to function types and specifies that the function shares
+ZA state with its caller.  This means that:
+
----------------
"ZA" should be defined 1st time it is used in this doc, and the ones below.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6635
+
+* the function requires the Scalable Matrix Extension (SME)
+
----------------
Same comments as above.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6679
+
+* the function requires the Scalable Matrix Extension (SME)
+
----------------
Same here.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6707
+
+* the function requires the Scalable Matrix Extension (SME)
+
----------------
again.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3758
 
+  // It is not allowed to redeclare an SME function with different SME
+  // attributes.
----------------



================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9444
+  case ParsedAttr::AT_ArmNewZA:
+    if (auto *FPT = dyn_cast<FunctionProtoType>(D->getFunctionType())) {
+      if (FPT->getAArch64SMEAttributes() &
----------------
This switch should ONLY be 'handle' function calls.  Please break this off into its own function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762