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

Tue Nov 22 01:58:41 PST 2022

sdesmalen added inline comments.

================
Comment at: clang/include/clang/AST/Type.h:3940
+    /// on declarations and function pointers.
+    unsigned AArch64SMEAttributes : 8;
+
----------------
erichkeane wrote:
> sdesmalen wrote:
> > erichkeane wrote:
> > > We seem to be missing all of the modules-storage code for these.  Since this is modifying the AST, we need to increment the 'breaking change' AST code, plus add this to the ASTWriter/ASTReader interface.
> > > Since this is modifying the AST, we need to increment the 'breaking change' AST code
> > Could you give me some pointers on what you expect to see changed here? I understand your point about adding this to the ASTWriter/Reader interfaces for module-storage, but it's not entirely clear what you mean by "increment the breaking change AST code". 
> > 
> > I see there is also an ASTImporter, I guess this different from the ASTReader?
> > 
> > Please apologise my ignorance here, I'm not as familiar with the Clang codebase.
> See VersionMajor here: https://clang.llvm.org/doxygen/ASTBitCodes_8h_source.html
> 
> Yes, ASTReader/ASTWriter and ASTImporter are different unfortunately.  I'm not completely sure of the difference, but doing this patch changing the type here without doing those will break modules.
So I tried to create some tests for this (see clang/test/AST/ast-dump-sme-attributes.cpp) and realised that the serialization/deserialization works out-of-the-box.

Does that mean all is needed is an increment of the VERSION_MAJOR or VERSION_MINOR number?

================
Comment at: clang/include/clang/AST/Type.h:4008
     bool HasTrailingReturn : 1;
+    unsigned AArch64SMEAttributes : 8;
     Qualifiers TypeQuals;
----------------
aaron.ballman wrote:
> erichkeane wrote:
> > sdesmalen wrote:
> > > 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
> > Are they aware that includes; `void Baz();` ?
> FWIW, the linked docs say:
> ```
> > Functions like `some_func` and `another_func` are referred to as
> > (K&R-style) “unprototyped” functions. The first C standard categorized
> > them as an obsolescent feature and C18 removed all remaining support
> > for them.
> ```
> That's not quite accurate. They've always been obsolete in standard C (both ANSI and ISO C), so that's correct. But it's C2x that removes all remaining support for them. (Also, there's no such release as C18, there's C11, C17, and expected to be C23).
> 
> > Are they aware that includes; void Baz(); ?
> 
> Just to expound on this question: this means `void baz();` in C17 mode cannot have the attribute but `void baz();` in C2x mode can. Folks coming from C++ tend to expect the C2x behavior to be the default and so it's plausible to see such signatures in older language modes. Will that cause any pain or confusion for you?
@rsandifo-arm explained to me that it is indeed a deliberate choice not to support the attribute on unprototyped functions, as these are an obsolescent feature that will be removed in C2x. The mitigation should be trivial for new code that is written and it makes new code compliant with the next version of the standard.

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

+def ArmStreamingCompatible : DeclOrTypeAttr, TargetSpecificAttr<TargetAArch64> {
+  let Spellings = [Clang<"arm_streaming_compatible">];
----------------
aaron.ballman wrote:
> sdesmalen wrote:
> > aaron.ballman wrote:
> > > sdesmalen wrote:
> > > > aaron.ballman wrote:
> > > > > sdesmalen wrote:
> > > > > > 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).```
> > > > > > 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.
> > > > > 
> > > > > What are the chances that can change? Because there are problems here:
> > > > > 
> > > > > > If the attribute is attached to a function declaration or definition, it applies to the type of the function.
> > > > > 
> > > > > This is egregiously opposed to the design of `[[]]` attributes in both C and C++. We came up with `DeclOrTypeAttr` for attributes that previously existed, but that is different than introducing new attributes. It's par for the course for `__attribute__` style attributes, so I don't worry so much there.
> > > > > 
> > > > > What's the rationale for this confusing of a design? (e.g., is there some reason you basically have to do that, like historically accepting the attributes in both positions?)
> > > > The attribute must always apply to the type of the function, because we can't have the streaming-property (or the properties on ZA) being lost between function overrides or function pointer assignments. It's perhaps similar to a calling convention, because the caller may have to set up streaming- or ZA state before the call, and restore state after the call.
> > > > 
> > > > I'm not too familiar with the different spellings/syntaxes and their implications, so I've now limited the attribute to `GNU` style attributes (`__attribute__((..))`) and to being a `TypeAttr`, with the exception of the `arm_locally_streaming` attribute, because that one can only be applied to function definitions and is not part of the type.
> > > > 
> > > > I've also added some new tests to ensure the properties are correctly propagated (using `decltype()`) and tests to ensure virtual function overloads require the same attributes.
> > > > 
> > > > Is this a step in the right direction? :)
> > > Switching to a GNU-only spelling sort of helps, but I still think this design is confused.
> > > ```
> > > *  If the attribute is attached to a function declaration or definition,
> > >     it applies to the type of the function.
> > > ```
> > > This seems like a misdesigned feature and I think it should be fixed in the specification. If it's an attribute that applies to the type of the function, you should not be allowed to specify it on the declaration; what is the benefit to allowing it in an unexpected position?
> > > If it's an attribute that applies to the type of the function, you should not be allowed to specify it on the declaration
> > Could you maybe give me an example here of what it looks like to apply the attribute to the function *type* when it's not allowed on the *declaration*? Does this all have to do with the //position// of the attribute in the declaration statement? When writing a function declaration it defines both the name and the type of that function, so there's no way to write a declaration without also defining its type.
> > 
> > Maybe it helps if you have any pointers that explain the difference in syntax between function declaration/function type attributes (https://clang.llvm.org/docs/AttributeReference.html doesn't really address this)
> > Could you maybe give me an example here of what it looks like to apply the attribute to the function *type* when it's not allowed on the *declaration*? Does this all have to do with the position of the attribute in the declaration statement? When writing a function declaration it defines both the name and the type of that function, so there's no way to write a declaration without also defining its type.
> 
> Sure, but I'm going to use `[[]]` syntax to demonstrate the difference because it's very hard to reason about *what* a GNU-style attribute applies to given it's "sliding" behavior. C++ attributes have rules about what they appertain to based on syntactic location of the attribute so it's an easier way to demonstrate.
> 
> https://godbolt.org/z/Yz37fTbWY demonstrates some of the differences in terms of positioning of the attribute.
> https://godbolt.org/z/18Y1Pn4se demonstrates the behavior when the type attribute matters for the type identify of the function.
> https://godbolt.org/z/1jTqPx4f4 demonstrates another way in which a type attribute can matter.
> 
> For `__attribute__` it's harder to reason about because of https://gcc.gnu.org/onlinedocs/gcc/extensions-to-the-c-language-family/attribute-syntax.html, especially because GCC and Clang don't always agree in terms of whether something can be used as a type attribute or not. But in terms of the mental model of attributes, the question really boils down to how much the attribute matters to the identity of the type -- if you don't want users to be able to assign an attributed function to a nonattributed function pointer (or vice versa), it most likely is a type attribute. If you think presence or absence of the attribute should impact things like overload resolution or template specializations, it's almost certainly a type attribute.
Thank you, those examples are very helpful!

================
Comment at: clang/include/clang/Basic/AttrDocs.td:6334-6336
+(ACLE) for SME. It is used to mark a function's body (not the interface) as requiring
+``PSTATE.SM`` to be ``1``, although the function is expected to be called with
+``PSTATE.SM=0`` and return with ``PSTATE.SM`` unchanged.
----------------
aaron.ballman wrote:
> I'm not certain I understand what this is telling me. The function is expected to be called with PSTATE.SM == 0 and return with it unchanged, so what does it mean for it to also require PSTATE.SM to be 1? I *think* it might be saying that the function is called with SM = 0 and returns with SM = 0, but within the function body itself SM will be 1?
Yes, that's what I was trying to describe, albeit in a slightly confusing way :) I've rephrased it!

================
Comment at: clang/include/clang/Basic/AttrDocs.td:6366-6367
+The ``arm_shared_za`` attribute is defined by the Arm C Language Extensions
+(ACLE) for SME. It is used to mark a function as sharing the state of ZA, the
+acculator array, with that of it's callers.
+
----------------
aaron.ballman wrote:
> Is `acculator` a typo or is that a term of art I am unfamiliar with?
That was a typo, well spotted.

================
Comment at: clang/include/clang/Basic/AttrDocs.td:6389-6396
+def ArmSmePreservesZADocs : Documentation {
+  let Category = DocCatType;
+  let Content = [{
+The ``arm_preserves_za`` attribute is defined by the Arm C Language Extensions
+(ACLE) for SME. If a function is marked as ``arm_preserves_za``, it is a hint to
+the compiler that the function and any of it's callees will preserve the state
+of ZA.
----------------
aaron.ballman wrote:
> If this is a hint, why is the attribute part of the type system? It seems like it'd be fine for the hint to be lost when forming a function pointer, as in:
> ```
> __attribute__((arm_preserves_za)) void func(void);
> 
> void foo(void) {
>   void (*fp)(void) = func; // Doesn't need to be an error, right?
> }
> ```
You're right, this should not lead to an error, the ACLE specification makes an exception for `arm_preserves_za`:
> Function types with this attribute implicitly convert to function types that do not have the attribute. However, the reverse is not true.

I've fixed that and adjusted the test accordingly (both conversions are tested)

================
Comment at: clang/lib/Basic/Targets/AArch64.cpp:589-590
+      HasSME = true;
+      HasBFloat16 = true;
+      HasFullFP16 = true;
+    }
----------------
aaron.ballman wrote:
> I don't see any test coverage for these changes, and it seems orthogonal -- should this be split into its own review?
This is actually tested in clang/test/Sema/aarch64-sme-attrs-no-sme.c, where it gives a warning diagnostic that the attribute is ignored when +sme is not specified.

================
Comment at: clang/lib/CodeGen/CGCall.cpp:2231-2247
+    if (TargetDecl->hasAttr<ArmStreamingAttr>())
+      FuncAttrs.addAttribute("aarch64_pstate_sm_enabled");
+
+    if (TargetDecl->hasAttr<ArmLocallyStreamingAttr>())
+      FuncAttrs.addAttribute("aarch64_pstate_sm_body");
+
+    if (TargetDecl->hasAttr<ArmStreamingCompatibleAttr>())
----------------
aaron.ballman wrote:
> The declaration won't have most of these attributes because they're type attributes, not declaration attributes. This looks like mostly dead code.
Good spot, I only need to handle `aarch64_pstate_sm_body` because that is a declaration attribute, not a type attribute.

================
Comment at: clang/lib/Sema/SemaDecl.cpp:3746-3753
+  // It is not allowed to redeclare an SME function with different SME
+  // attributes.
+  if (IsInvalidSMECallConversion(Old->getType(), New->getType())) {
+    Diag(New->getLocation(), diag::err_sme_attr_mismatch)
+        << New->getType() << Old->getType();
+    Diag(OldLocation, diag::note_previous_declaration);
+    return true;
----------------
aaron.ballman wrote:
> How should these work with template (partial) specializations? Should it be invalid to write this on the primary template? Should it be invalid if the specialization doesn't match the primary template?
Can you share an example of what you're thinking of?

Do you mean something like:

  template<typename T> void (*a)(T, T);
  template<> __attribute__((arm_streaming)) void (*a<int>)(int, int);

where calling `a<short>(0, 0)` and `a<int>(0, 0)` have different streaming-mode behaviour? If so, then that is the intent. The SME attributes behave similar to a calling convention attributes, with the difference that more than one attribute can be applied.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:278
     S.Diag(A->getLocation(), diag::note_conflicting_attribute);
+    AL.setInvalid();
     return true;
----------------
aaron.ballman wrote:
> I think this is likely a good change but it seems orthogonal to the rest of the patch (it'd be helpful to more clearly see what test behavior this changes).
It turns out this change is no longer needed, I'll remove it.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127762