Page MenuHomePhabricator

[Clang][AArch64] Add ACLE attributes for SME.
Needs ReviewPublic

Authored by sdesmalen on Jun 14 2022, 9:32 AM.

Details

Summary

This patch adds all the language-level function attributes defined in:

https://github.com/ARM-software/acle/pull/188

The attributes are used to control PSTATE.ZA and PSTATE.SM, which are
respectively used for enabling the use of the ZA matrix array and
Streaming mode. This information needs to be available on call sites,
since the use of ZA or streaming mode may have to be enabled or disabled
around the call-site (depending on the attributes set on the caller and the
callee). For calls to functions from a function pointer, there is no IR
declaration available, so the attributes must be added explicitly to the
call-site.

With the exception of 'arm_locally_streaming', the information is part of
the function's interface, not just the function definition, and thus needs
to be propagated through the FunctionProtoType::ExtProtoInfo.

This patch adds the defintions of these attributes, as well as semantic
analysis to ensure conversions between function pointers are valid and
that no conflicting attributes are set. For example, 'arm_streaming' and
'arm_streaming_compatible' are mutually exclusive.

LLVM support for these attributes will follow later, so at the moment
these attributes are non-functional.

Diff Detail

Event Timeline

sdesmalen created this revision.Jun 14 2022, 9:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 9:33 AM
sdesmalen requested review of this revision.Jun 14 2022, 9:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 9:33 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

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?

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.

clang/include/clang/AST/Type.h
3806–3811

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

4008

So functions without prototypes cannot have any of these attributes?

clang/include/clang/Basic/Attr.td
2325

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?

2328

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).

clang/lib/Sema/SemaDeclAttr.cpp
8091–8103

I think this should be handled declaratively in Attr.td by defining a new TargetArch that checks this property, similar to: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Basic/Attr.td#L387

8109–8112

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

9060–9067

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.

clang/lib/Sema/SemaType.cpp
7669–7672

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).

7676

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.

clang/test/Sema/aarch64-sme-attrs-no-sme.c
2

I assume that option wasn't necessary for the test?

4
clang/test/Sema/aarch64-sme-attrs-on-x86.c
1 ↗(On Diff #436823)

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.

clang/test/Sema/aarch64-sme-func-attrs.c
2

Why are these defines needed?

90

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) {}
108

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]];
168

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

sdesmalen updated this revision to Diff 437580.Jun 16 2022, 9:50 AM
sdesmalen marked 10 inline comments as done.

Addressed bunch of the review comments (though not all yet).

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.

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.

clang/include/clang/AST/Type.h
3806–3811

Unless I'm misunderstanding its purpose, I'm not sure if using BitmaskEnum is worth it, because there's only little casting going on.

4008

Yes, the ACLE explicitly states that here:

The function type attributes cannot be used with K&R-style “unprototyped” C function types

clang/include/clang/Basic/Attr.td
2325

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 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).
2328

Fair point, I've added some documentation.

clang/lib/Sema/SemaDeclAttr.cpp
8109–8112

Nice suggestion, I wasn't aware of that!

9060–9067

marking this as done, because they should be declaration attributes as well (see my other comment).

clang/lib/Sema/SemaType.cpp
7669–7672

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?

7676

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.

clang/test/Sema/aarch64-sme-attrs-no-sme.c
2

Good catch, thanks!

clang/test/Sema/aarch64-sme-attrs-on-x86.c
1 ↗(On Diff #436823)

Great tip!

clang/test/Sema/aarch64-sme-func-attrs.c
2

You're right, they weren't needed.

90

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);

?

108

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));

?

168

Okay, I'll add some more tests, thanks for the suggestions! (not marking your suggestion as 'Done' yet until I've added these tests)

sdesmalen updated this revision to Diff 438624.Jun 21 2022, 3:14 AM
  • Increased test-coverage by adding positive tests for:
    • template instantiations
    • function overloading
    • lambda function with attribute
    • (indirect) pointer to pointer to an attributed function type.
  • Also added negative tests for:
    • passing an operand to an arm_streaming attribute
    • Adding an arm_streaming attribute to a non-function type.
  • Rewritten the CHECK lines for aarch64-sme-attrs.cpp, such that the attributes are checked with named labels, e.g. #[[SM_ENABLED_CALL]].

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.

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.

Good to know, thank you for the details!

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.

SGTM!

clang/include/clang/Basic/Attr.td
2325

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?)

clang/lib/Sema/SemaType.cpp
7676

Yeah, I can see why we'd hit that problem -- when we go to form the function type to be able to make the declaration, we'd issue the warning once, and then after forming the declaration and trying to apply attributes to it, we'd issue the warning a second time. We do not have any other DeclOrTypeAttr which has mutual exclusions.

I don't want to lose the declarative nature of using MutualExclusions in Attr.td unless there's no other choice; that's a helpful piece of documentation to reviewers when reviewing additional attributes in the future. If you elect to keep the attribute as a declaration and type attribute, I think you'll need to try to fix this to work properly.

clang/test/Sema/aarch64-sme-func-attrs.c
90

Those test applying mutually exclusive attributes to unique declarations, but I was asking for applying mutually exclusive attributes to redeclarations.

108

What exactly do you mean with 'declaration attributes' here? Are these not added to the type fptrty9 in the test above?

I had misread the code as if the attribute was being applied to the function declaration on the next line down. They are added to the type.

Or is that the difference between having

No difference with the GNU spelling, sorry for the noise!

sdesmalen updated this revision to Diff 439330.Jun 23 2022, 4:31 AM
sdesmalen marked 7 inline comments as done.
  • Limited attribute to GNU spelling attribute((...))
  • Changed arm_locally_streaming attribute to be DeclOrStmtAttr because it does not apply to type (only the definition)
  • Changed the other SME attributes to be TypeAttr.
  • Added checks and new tests for conflicting attributes on redeclarations
  • Added checks and new tests for conflicting attributes on virtual function overrides
  • Added new tests for propagation of type attributes on other declarations using decltype(otherty)
sdesmalen added inline comments.Jun 23 2022, 4:33 AM
clang/include/clang/Basic/Attr.td
2325

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? :)

clang/lib/Sema/SemaType.cpp
7676

By changing the attributes to become TypeAttr, the MutualExclusions no longer kick in at all (you mentioned this in a previous comment), so I've had to keep the manual code to check the attributes here. Are you okay with that?

clang/test/Sema/aarch64-sme-func-attrs.c
90

Good point, I've had to add some new checks to ensure we get a diagnostic when compiling for C. For C++ it already gave a conflicting types for <function> diagnostic. I'm not really sure why that behaviour is different between C and C++.

aaron.ballman added inline comments.Jun 27 2022, 10:26 AM
clang/include/clang/AST/Type.h
4064

If we're taking up space in the extra bitfields, why do we need to also take up space in the function prototype itself?

clang/include/clang/Basic/Attr.td
2325

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?

2333

You are handling these as declaration attributes in SemaDeclAttr.cpp but declaring them to be type attributes here in Attr.td; that's not okay. If these are type attributes, there should not be code for them in SemaDeclAttr.cpp. (This is why I really think the design needs to be rethought; I think they should be type attributes only, but I think you're trying to match what your specification says.)

2345

Why is this a *statement* attribute?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2000 ↗(On Diff #439330)

This error and the one below make me wonder whether an attribute spelling is the appropriate way to surface this functionality as opposed to a keyword. Attributes don't typically don't cause errors in these situations, but because these attributes are strongly coupled to their type system effects I can see why you want these to be errors.

3542–3543 ↗(On Diff #439330)

No trailing full stop in Clang diagnostics

clang/lib/Sema/SemaType.cpp
7676

Yeah, that is reasonable, thanks for checking it!

clang/test/Sema/aarch64-sme-func-attrs.c
214

This is an interesting start, but here are some more cases:

template <typename Ty>
struct S {
  static constexpr int value = 0;
};

template <>
struct S<void (*)()> {
  static constexpr int value = 1;
};

template <>
struct S<void (* __attribute__((arm_streaming)))()> {
  static constexpr int value = 2;
};

void func() __attribute__((arm_streaming)) {}
void other_func() {}

static_assert(C<decltype(+func)>::value == 2, "why are we picking the wrong specialization?");
static_assert(C<decltype(+other_func)>::value == 1, "why are we picking the wrong specialization?");

another interesting test would be to ensure that implicit instantiations do not lose the attribute from the primary template for codegen:

template <typename Ty>
void func(Ty) __attribute__((arm_streaming)) {}

int main() {
  func(12); // Does this properly insert the correct IR instructions?
}
sdesmalen updated this revision to Diff 440631.Jun 28 2022, 8:01 AM
sdesmalen marked 3 inline comments as done.
  • Removed attribute handling from SemaDeclAttr.cpp, so that the type attributes are only handled in SemaType.cpp
  • Made ArmLocallyStreaming a DeclOrTypeAttribute again.
  • Added new tests.

Thanks for your patience reviewing this patch @aaron.ballman!

clang/include/clang/AST/Type.h
4064

This field is added to ExtProtoInfo struct, which is used to populate the ExtraBitfields in the FunctionPrototype's AST node.

Sorry if this wasn't clear because the diff had no context. arc diff wasn't working for me, so manually updated the patch, but then forgot to add full context.

clang/include/clang/Basic/Attr.td
2325

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)

2333

Okay, I've removed the handling in SemaDeclAttr, so that it is only handled in SemaType.cpp.

But it still allows the attribute to be applied to any position in the function declaration, i.e.

__attribute__((arm_streaming)) float foo1(int);
float __attribute__((arm_streaming)) foo2(int);
float foo3(int) __attribute__((arm_streaming));

Is that expected?

2345

You're right that it shouldn't be. I want it to just be a decl attribute and don't want it to be either a Type or a Stmt attribute, but that doesn't seem to exist.
I find their meaning quite confusing, because both allow me to limit their applicability to Functions. What is the right class to derive from here?

clang/include/clang/Basic/DiagnosticSemaKinds.td
2000 ↗(On Diff #439330)

This error and the one below make me wonder whether an attribute spelling is the appropriate way to surface this functionality as opposed to a keyword.

I'm not sure I understand what you mean, do you have an example?

clang/test/Sema/aarch64-sme-func-attrs.c
214

Great suggestions, thanks! They both seem to be handled correctly.

Thanks for your patience reviewing this patch @aaron.ballman!

Happy to help, thank you for your patience with my constant stream of questions about the design. :-)

clang/include/clang/AST/Type.h
4064

Ah, I see what's going on there, thank you (the extra context helped as well).

clang/include/clang/Basic/Attr.td
2333

https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html#Attribute-Syntax

"For compatibility with existing code written for compiler versions that did not implement attributes on nested declarators, some laxity is allowed in the placing of attributes. If an attribute that only applies to types is applied to a declaration, it is treated as applying to the type of that declaration. If an attribute that only applies to declarations is applied to the type of a declaration, it is treated as applying to that declaration; and, for compatibility with code placing the attributes immediately before the identifier declared, such an attribute applied to a function return type is treated as applying to the function type, and such an attribute applied to an array element type is treated as applying to the array type. If an attribute that only applies to function types is applied to a pointer-to-function type, it is treated as applying to the pointer target type; if such an attribute is applied to a function return type that is not a pointer-to-function type, it is treated as applying to the function type."

So yes, that's expected because the attribute only applies to types, so it magically "slides" onto the type regardless of where it was written on the declaration. By making it a type only attribute, it's now clear what the GNU attribute applies to regardless of where it's written.

2345

Hmm, we have some documentation at https://clang.llvm.org/docs/InternalsManual.html#how-to-add-an-attribute but it's not very clear on all the choices and when to pick which one.

For declaration attributes on a function, the question really boils down to whether you want redeclarations to automatically inherit the attribute or not. e.g.,

__attribute__((arm_locally_streaming)) void func(void);
void func(void); // Is this declaration also marked locally streaming?

void use(void) {
  func(); // Does this use see the attribute or not?
}

If you want the call in use() to see the attribute, then you want the attribute to be inherited on the redeclaration, and so you'd use InheritableAttr. If you don't want the call in use() to see the attribute, then you'd use Attr.

clang/include/clang/Basic/DiagnosticSemaKinds.td
2000 ↗(On Diff #439330)

override and final could have been surfaced via attributes, and in Clang we semantically express them as such (see Final and Override in Attr.td), but instead they are surfaced to the user as keywords in the language standard. You're not under the same constraints as the standard (you're making a vendor-specific attribute). We're not super consistent with whether we use the same approach as the standard (we have some type attributes that are spelled as attributes like vector_size and others that are spelled via keywords like _Nullable.

So you could spell your type attributes the way you have been with __attribute__, or you could come up with keywords for them (so instead of using GNU<"whatever"> for the attribute, you could use Keyword<_Whatever> or Keyword<__whatever> (you'd also need to add them as recognized keyword tokens, parse them as appropriate, etc).

Note: I don't insist on you using a keyword for these, but I am wondering if that approach was considered or not given how non-portable the attributes are (if an implementation ignores this attribute, it sounds like the program semantics are unlikely to be correct).

clang/lib/Sema/SemaType.cpp
7669–7672

I'm sorry, I see now that I forgot to say the important bit that was in my head: we don't check those subjects automatically yet, but we do want to add more automation to type attribute processing as we've already done for declarations and statements, so we want the content in Attr.td to be as correct as possible even though it still needs to be manually checked. That makes it easier on us when we go to add the automation; there's less "which is correct, the SemaType.cpp code or the Attr.td declaration?" involved).