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

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

Thanks both for the detailed review and latest round of comments, I've tried to address them all.



================
Comment at: clang/include/clang/AST/Type.h:3987
     /// [implimits] 8 bits would be enough here.
-    uint16_t NumExceptionType = 0;
+    uint16_t NumExceptionType;
+
----------------
aaron.ballman wrote:
> 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.
That's what I did initially in the patch where I limited `NumExceptionType` to 16bits (D152140), but there was the preference to have this be a uint16_t instead. I've changed it back again to a bitfield though.


================
Comment at: clang/include/clang/Basic/Attr.td:2439
 
 def ArmStreaming : TypeAttr, TargetSpecificAttr<TargetAArch64SME> {
   let Spellings = [RegularKeyword<"__arm_streaming">];
----------------
rsandifo-arm wrote:
> 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.)
That's a fair point. This change was partially made in D152141, so I've updated both patches to reflect the right behaviour.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6318
+def ArmSmeStreamingDocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
----------------
aaron.ballman wrote:
> There are enough of these attributes that I think we should consider adding an `ARM C Language Extensions for SME` documentation category:
> ```
> def DocCatACLE_SME : DocumentationCategory<"ARM C Language Extensions for SME"> {
>   let Content = [{
> Put general information about what ACLE SME is here to give an overview of the functionality and a place to link to the official ACLE SME documentation for more in-depth details.
> }];
> }
> ```
> and then use this for the `Category` for each of the attributes so they group together.
@rsandifo-arm pointed out that I marked this comment as done, but hadn't done it. I've fixed that now, and created a new category for the SME extensions.


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


================
Comment at: clang/lib/AST/TypePrinter.cpp:943
+       FunctionType::SME_PStateSMCompatibleMask) &&
+      !InsideCCAttribute)
+    OS << " __arm_streaming_compatible";
----------------
rsandifo-arm wrote:
> 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.
You're right, they were not necessary! Thanks.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:9688
+      Changed == FunctionType::SME_PStateZAPreservedMask &&
+      (ToAttributes & FunctionType::SME_PStateZAPreservedMask);
+  return !DropsPreservesZA;
----------------
rsandifo-arm wrote:
> 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.
Thanks for pointing out, that was indeed wrong. I've fixed it, and added a test-case for the virtual function override for the __arm_preserves_za case.


================
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,
----------------
rsandifo-arm wrote:
> Is it not possible to address this FIXME yet?  Might be worth expanding the comment to say why if so.
This was a stale comment, these cases are checked later on in section:

  // 
  // Check for incorrect conversions of function pointers with the attributes
  // 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762