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

Thu Jun 16 09:50:33 PDT 2022

sdesmalen added a comment.

Thanks @aaron.ballman for your elaborate review, that was very helpful! I'm still working through some of your suggestions (although some of them weren't entirely clear to me), but I've addressed a number of them already.

In D127762#3582753 <https://reviews.llvm.org/D127762#3582753>, @aaron.ballman wrote:

>> This patch adds all the language-level function attributes defined in:
>>
>> https://github.com/ARM-software/acle/pull/188
>
> This pull request has not yet been merged, so what are the chances that it gets rejected or altered?

There has already been quite some discussion on the draft both online and offline, and we already started to implement it to get confidence that there's no big flaws in its design. Other than perhaps some minor changes, I expect it to land in its current form.

>> LLVM support for these attributes will follow later, so at the moment these attributes are non-functional.
>
> FWIW, I would rather we land the attributes once they're functional even if we do a bunch of the review work up front. While rare, we have run into the situation where attributes get added with intentions to make them useful later but then something comes up and we're left with half the implementation exposed to users.

I think that's fair. We'll share LLVM support for these attributes shortly, but I'm happy to wait landing this work until the LLVM support has landed. It would be nice if I can get this patch in a state where you and other reviewers are happy with it, so that it only needs a quick rebase once the LLVM support is there.

================
Comment at: clang/include/clang/AST/Type.h:3806-3811
+    SME_NormalFunction = 0,
+    SME_PStateSMEnabledMask = 1,
+    SME_PStateSMCompatibleMask = 2,
+    SME_PStateZANewMask = 4,
+    SME_PStateZASharedMask = 8,
+    SME_PStateZAPreservedMask = 16,
----------------
aaron.ballman wrote:
> Also, would it be better to use a bitmask enumeration here? https://github.com/llvm/llvm-project/blob/main/llvm/include/llvm/ADT/BitmaskEnum.h
Unless I'm misunderstanding its purpose, I'm not sure if using BitmaskEnum is worth it, because there's only little casting going on.

================
Comment at: clang/include/clang/AST/Type.h:4008
     bool HasTrailingReturn : 1;
+    unsigned AArch64SMEAttributes : 8;
     Qualifiers TypeQuals;
----------------
aaron.ballman wrote:
> So functions without prototypes cannot have any of these attributes?
Yes, the ACLE explicitly states that [[https://github.com/ARM-software/acle/pull/188/commits/59751df91d9630400531a64108f179e3951c3b89#diff-516526d4a18101dc85300bc2033d0f86dc46c505b7510a7694baabea851aedfaR503|here]]:
> The function type attributes cannot be used with K&R-style “unprototyped” C function types

================
Comment at: clang/include/clang/Basic/Attr.td:2322

+def ArmStreamingCompatible : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64> {
+  let Spellings = [Clang<"arm_streaming_compatible">];
----------------
aaron.ballman wrote:
> `DeclOrTypeAttr` is only intended to be used for attributes which were historically a declaration attribute but are semantically type attributes (like calling conventions). It seems to me that each of these is purely a type attribute and we shouldn't allow them to be written in the declaration position. WDYT?
In this case, I believe the attribute is valid on both the type and the declaration, because the SME ACLE explicitly specifies that the attributes can be applied to both declarations and types.

The [[https://github.com/ARM-software/acle/pull/188/commits/ce878cf6c43fd824ffc634526e5dfec1ddc1839e#diff-516526d4a18101dc85300bc2033d0f86dc46c505b7510a7694baabea851aedfaR8283-R8294|specification]] says:
```Some of the attributes described in this section apply to function types.
Their semantics are as follows:

*   If the attribute is attached to a function declaration or definition,
    it applies to the type of the function.

*   If the attribute is attached to a function type, it applies to that type.

*   If the attribute is attached to a pointer to a function type, it applies
    to that function type.

*   Otherwise, the attribute is [ill-formed](#ill-formed).```

================
Comment at: clang/include/clang/Basic/Attr.td:2325
+  let Subjects = SubjectList<[FunctionLike], ErrorDiag>;
+  let Documentation = [Undocumented];
+}
----------------
aaron.ballman wrote:
> No new, undocumented attributes (same goes for the rest of the additions); please add some docs to AttrDocs.td (it's okay to lightly document and point to a canonical resource for more details).
Fair point, I've added some documentation.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8109-8112
+  // Handle mutually exclusive SME function attributes:
+  //  -  arm_streaming & arm_streaming_compatible
+  //  -  arm_new_za & arm_preserves_za
+  //  -  arm_new_za & arm_shared_za
----------------
aaron.ballman wrote:
> This should be handled declaratively in Attr.td, like this: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L1403
Nice suggestion, I wasn't aware of that!

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:9078-9085
+  case ParsedAttr::AT_ArmStreaming:
+  case ParsedAttr::AT_ArmStreamingCompatible:
+  case ParsedAttr::AT_ArmLocallyStreaming:
+  case ParsedAttr::AT_ArmSharedZA:
+  case ParsedAttr::AT_ArmPreservesZA:
+  case ParsedAttr::AT_ArmNewZA:
+    handleSMEAttrs(S, D, AL);
----------------
aaron.ballman wrote:
> If the attributes are no longer declaration attributes, then all of this can go away because it needs to be handled from SemaType.cpp instead.
marking this as done, because they should be declaration attributes as well (see my other comment).

================
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.
----------------
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?

================
Comment at: clang/lib/Sema/SemaType.cpp:7676
+      return false;
+    }
+
----------------
aaron.ballman wrote:
> Unfortunately, type attributes do not yet have any of the automated checking generated by tablegen, so there are no calls to `Sema::checkCommonAttributeFeatures()`, which means you have to manually check things like mutual exclusions, not accepting arguments, etc.
After some experimenting I found that:
* When I add the `MutualExclusions` to Attr.td and compile `typedef __attribute__((arm_streaming, arm_streaming_compatible)) void (*ptrty1) (void);`, I still get the diagnostic for conflicting attributes 'for free', even without any code being added here to check for conflicting attributes. However, when I then apply it to a declaration (e.g. `void __attribute__((arm_streaming, arm_streaming_compatible)) foo(void);`), I get the diagnostics twice.
* When I add some code here to mark the attribute as invalid, I get no diagnostic whatsoever unless I explicitly emit it here, rendering the use of `MutualExclusions` in Attr.td ineffective.

Based on the above observation, I've chosen to keep the code in SemaDeclAttr.cpp and not use the `MutualExclusions`, because it generates the best diagnostics (an error + a note) and only emits it once. Let me know what you think.

================
Comment at: clang/test/Sema/aarch64-sme-attrs-no-sme.c:1
+// RUN: %clang_cc1 -triple aarch64-none-linux-gnu -fallow-half-arguments-and-returns -fsyntax-only -verify %s
+
----------------
aaron.ballman wrote:
> I assume that option wasn't necessary for the test?
Good catch, thanks!

================
Comment at: clang/test/Sema/aarch64-sme-attrs-on-x86.c:1
+// RUN: %clang_cc1 -triple x86_64-none-linux-gnu -target-feature +sme -fallow-half-arguments-and-returns -fsyntax-only -verify %s
+
----------------
aaron.ballman wrote:
> I think this RUN line should be added to the previous test since the test contents are the same and it's just the diagnostic behavior being tested. You can use `-verify=foo` and `foo-warning {{}}`/`foo-error {{}}` to differentiate between the diagnostics if you need to.
Great tip!

================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:1
+// RUN: %clang_cc1 -D__ARM_FEATURE_SME=1 -D__ARM_FEATURE_LOCALLY_STREAMING=1 \
+// RUN:   -triple aarch64-none-linux-gnu -target-feature +sme -fsyntax-only -verify %s
----------------
aaron.ballman wrote:
> Why are these defines needed?
You're right, they weren't needed.

================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:89
+// expected-note at +2 {{conflicting attribute is here}}
+__attribute__((arm_preserves_za, arm_new_za)) void preserves_new_za(void);
+
----------------
aaron.ballman wrote:
> If you keep the attribute as a declaration attribute, you're also missing tests like:
> ```
> __attribute__((arm_new_za)) void func(void);
> __attribute__((arm_preserves_za)) void func(void) {}
> ```
> 
I'm not sure what you mean, there are tests for:
* `__attribute__((arm_new_za)) void sme_arm_new_za(void);`
* `__attribute__((arm_preserves_za)) void sme_arm_preserves_za(void);`
?

================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:107
+typedef __attribute__((arm_new_za, arm_shared_za)) void (*fptrty9) (void);
+fptrty9 invalid_shared_za_func() { return shared_za_ptr_invalid; }
+
----------------
aaron.ballman wrote:
> These are all being used as declaration attributes, I think you also need a test like:
> ```
> void (*fp)(void) [[clang::arm_new_za]] [[clang::arm_preserves_za]];
> ```
> 
What exactly do you mean with 'declaration attributes' here? Are these not added to the type `fptrty9` in the test above?

Or is that the difference between having

  typedef __attribute__((arm_new_za, arm_shared_za)) void (*fptrty9) (void);

and

  typedef void (*fptrty9) (void) __attribute__((arm_new_za, arm_shared_za));

?

================
Comment at: clang/test/Sema/aarch64-sme-func-attrs.c:167
+// expected-warning at +1 {{incompatible function pointer types returning 'pz_ptrty' (aka 'void (*)(void) __attribute__((arm_preserves_za))') from a function with result type 'n_ptrty' (aka 'void (*)(void)')}}
+n_ptrty return_invalid_fptr_normal_preserves_za(pz_ptrty f) { return f; }
----------------
aaron.ballman wrote:
> You're also missing test coverage for trying to apply the attributes to something other than a function and providing arguments to the attribute.
> 
> Also, you're missing significant test coverage for C++ where the type system effects are far more interesting. For example, tests that demonstrate you can overload based on the presence of the attribute, tests for template specialization behavior, adding the attribute to a lambda and calling it, etc. There's also some interesting questions there like "can you specify these attributes on a coroutine function?".
Okay, I'll add some more tests, thanks for the suggestions! (not marking your suggestion as 'Done' yet until I've added these tests)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762