[AArch64] Function multiversioning support added.

[ilinpv]

Summary

The patch adds new "target_version" function attribute and expands existing
"target_clones" one. This is initial implementation of Function Multi Versioning for AArch64
target following Arm C Language Extensions specification ( currently in Beta state )
https://github.com/ARM-software/acle/blob/main/main/acle.md#function-multi-versioning

It is not final yet, any comments, usage feedback, suggestions for improving are very welcome!

[MaskRay]

clang/docs/ClangCommandLineReference.rst

1086 ↗

(On Diff #436971)

This file is auto-generated. Don't touch it.

clang/lib/CodeGen/CGBuiltin.cpp

1173–1174

This can be committed separately.

clang/test/CodeGen/attr-target-version.c

76

Remove //. and // <empty>.

Note: some editors like neovim supports { } to navigate among paragraphs. Adding // just adds inconvenience.

[erichkeane]

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

clang/include/clang/AST/ASTContext.h

3090

It is concerning that this differs from the above.

clang/include/clang/AST/Decl.h

1860

I find myself immediately wondering why this isn't just 'Target'. What semantically are we trying to change?

clang/include/clang/Basic/Attr.td

2746

would probably just call this getName ?

clang/include/clang/Basic/AttrDocs.td

2351

This sounds absurdly like 'target', and I think we should instead investigate why enabling THAT for ARM isn't sufficient.

2374

This seems like an unnecessary change, we should do what we can to make this work as much like the non-ARM version as possible.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11490

I'm not clear as to what this means?

clang/include/clang/Sema/Sema.h

4501

This further concerns me that we're changing behavior this much...

[ilinpv]

In D127812#3585249, @erichkeane wrote:

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts. "target" attribute has been already used and supported on AArch64 in a different sense, like target("arm"), target("dotprod"), target("branch-protection=bti"). The intention of creating new "target_version" attribute is not to overlap with that. It also has different format, mangling and semantic, e.g. treating function without attribute as "default", and option to disable attribute droping function multi versions. Do these explanations dispel your concern?

[ilinpv]

clang/docs/ClangCommandLineReference.rst

1086 ↗

(On Diff #436971)

It looked out of sync with options td files:

+.. option:: -gen-reproducer=<arg>, -fno-crash-diagnostics (equivalent to -gen-reproducer=off)
+
+Emit reproducer on (option: off, crash (default), error, always)
+

+.. option:: -print-diagnostic-options, --print-diagnostic-options
+
+Print all of Clang's warning options
+

+.. option:: -fdriver-only
+
+Only run the driver.
+

...

[ilinpv]

clang/include/clang/AST/ASTContext.h

3090

target_version supports features only, that is similar to ParsedTargetAttr { std::vector<std::string> Features; }.

clang/include/clang/Basic/DiagnosticSemaKinds.td

11490

It gives a warning if target_clones attributes contains features which have no impact on code generation ( no supported yet ) and ignored. They has "<NS>" OPTION in llvm/include/llvm/Support/AArch64TargetParser.def
See clang/test/Sema/attr-target-clones-aarch64.c tests

// expected-warning@+1 {{version list contains no code impact entries}}
void __attribute__((target_clones("sha1+pmull"))) warn2(void);

// expected-warning@+1 {{version list contains no code impact entries}}
int __attribute__((target_clones("rng", "fhm+dpb+sha1", "default"))) redecl4(void) { return 1; }

[ilinpv]

Addressing comments, stylistic corrections.

[xbolva00]

clang/docs/ClangCommandLineReference.rst

1086 ↗

(On Diff #436971)

But it needs to be manually autogenerated..

[ilinpv]

clang/docs/ClangCommandLineReference.rst

1086 ↗

(On Diff #436971)

Done https://reviews.llvm.org/D128116, thanks to @MaskRay

[aaron.ballman]

In D127812#3587223, @ilinpv wrote:

In D127812#3585249, @erichkeane wrote:

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts.

Is it *necessary* that it diverges like this? (Is there some published standards document you're trying to conform to, is there an implementation difficulty with not diverging, something else?)

"target" attribute has been already used and supported on AArch64 in a different sense, like target("arm"), target("dotprod"), target("branch-protection=bti"). The intention of creating new "target_version" attribute is not to overlap with that. It also has different format, mangling and semantic, e.g. treating function without attribute as "default", and option to disable attribute droping function multi versions. Do these explanations dispel your concern?

Do I understand correctly that AArch64 was using an attribute named target which does not behave like the attribute with the same name in GCC and Clang on other architectures, and now you'd like to introduce a new attribute which does behave like target but under a different name? If I have that correct, I don't think that's reasonable -- there's far too much possibility for confusion with that situation already, and adding a new attribute only increases the confusion. I'm not certain where the technical debt came from, but we shouldn't increase the burden on users here; I think target and target_clones should be made to work consistently across architectures if at all possible.

[danielkiss]

In D127812#3599530, @aaron.ballman wrote:

In D127812#3587223, @ilinpv wrote:

In D127812#3585249, @erichkeane wrote:

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts.

Is it *necessary* that it diverges like this? (Is there some published standards document you're trying to conform to, is there an implementation difficulty with not diverging, something else?)

In ACLE there are a few rules/behaviour documented around what should be the behaviour of the "default" for example. Making for example "default" optional hopefully makes the adation of multi versioning seamless as possible. Compilers won't support from day one these attributes therefore the goal was to make the whole addition of a multi versioned function as less distributive as possible while the code is still compiled with older compilers too.

// this is the original code
// mandating __attribute__ ((target ("default"))) would not work with the today's compilers
void foo(){}

// new backward compatible code
#ifdef __ARM_FEATURE_FUNCTION_MULTI_VERSIONING
__attribute__ ((target_version("fancy_feature")))
void foo(){}
#endif

if the "default" is not mandated here, it felt not right to mandate it for the target_clones either.

"target" attribute has been already used and supported on AArch64 in a different sense, like target("arm"), target("dotprod"), target("branch-protection=bti"). The intention of creating new "target_version" attribute is not to overlap with that. It also has different format, mangling and semantic, e.g. treating function without attribute as "default", and option to disable attribute droping function multi versions. Do these explanations dispel your concern?

Do I understand correctly that AArch64 was using an attribute named target which does not behave like the attribute with the same name in GCC and Clang on other architectures, and now you'd like to introduce a new attribute which does behave like target but under a different name? If I have that correct, I don't think that's reasonable -- there's far too much possibility for confusion with that situation already, and adding a new attribute only increases the confusion. I'm not certain where the technical debt came from, but we shouldn't increase the burden on users here; I think target and target_clones should be made to work consistently across architectures if at all possible.

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

[erichkeane]

In D127812#3601476, @danielkiss wrote:

In D127812#3599530, @aaron.ballman wrote:

In D127812#3587223, @ilinpv wrote:

In D127812#3585249, @erichkeane wrote:

I'm concerned as to the design of this addition, I don't particularly appreciate the reasons for making 'target_clones' different, nor the purpose for adding a new attribute instead of using 'target' for what seems like exactly that? IF the new spelling is THAT necessary, we perhaps don't need a whole new attribute for it either.

Thank you for fair concern, "target_clones" for AArch64 has different format, semantic, e.g. "default" is not required. Therefore it diverges with X86 in these parts.

Is it *necessary* that it diverges like this? (Is there some published standards document you're trying to conform to, is there an implementation difficulty with not diverging, something else?)

In ACLE there are a few rules/behaviour documented around what should be the behaviour of the "default" for example. Making for example "default" optional hopefully makes the adation of multi versioning seamless as possible. Compilers won't support from day one these attributes therefore the goal was to make the whole addition of a multi versioned function as less distributive as possible while the code is still compiled with older compilers too.

// this is the original code
// mandating __attribute__ ((target ("default"))) would not work with the today's compilers
void foo(){}

// new backward compatible code
#ifdef __ARM_FEATURE_FUNCTION_MULTI_VERSIONING
__attribute__ ((target_version("fancy_feature")))
void foo(){}
#endif

if the "default" is not mandated here, it felt not right to mandate it for the target_clones either.

"target" attribute has been already used and supported on AArch64 in a different sense, like target("arm"), target("dotprod"), target("branch-protection=bti"). The intention of creating new "target_version" attribute is not to overlap with that. It also has different format, mangling and semantic, e.g. treating function without attribute as "default", and option to disable attribute droping function multi versions. Do these explanations dispel your concern?

Do I understand correctly that AArch64 was using an attribute named target which does not behave like the attribute with the same name in GCC and Clang on other architectures, and now you'd like to introduce a new attribute which does behave like target but under a different name? If I have that correct, I don't think that's reasonable -- there's far too much possibility for confusion with that situation already, and adding a new attribute only increases the confusion. I'm not certain where the technical debt came from, but we shouldn't increase the burden on users here; I think target and target_clones should be made to work consistently across architectures if at all possible.

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

I don't think differentiating the uses here is a good idea. I think it would have been a GREAT idea about 10 years ago, but that ship has already sailed once GCC started using it that way however. We should be keeping the current behavior, otherwise we're going to have a horrible mix of target/target_version working inconsistently between platforms.

[aaron.ballman]

In D127812#3602645, @erichkeane wrote:

In D127812#3601476, @danielkiss wrote:

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

I don't think differentiating the uses here is a good idea. I think it would have been a GREAT idea about 10 years ago, but that ship has already sailed once GCC started using it that way however. We should be keeping the current behavior, otherwise we're going to have a horrible mix of target/target_version working inconsistently between platforms.

That largely is my concern as well. The existing behavior of target is just that -- the existing behavior. I think deviating from that existing behavior will be confusing in practice. Adding additional attributes doesn't improve that confusion because users then have to know to decide between two very similar attributes, which means they then need to educate themselves on the differences between them. If we're going to push them towards the documentation to correctly use the attribute anyway, that's basically the same situation they're in today with the confusing dual behavior of target.

[samtebbs]

clang/include/clang/Basic/DiagnosticSemaKinds.td

11490

Perhaps it would be better to warn with something like "version list contains entries that don't impact code generation". I think that would be more clear.

clang/include/clang/Basic/TargetInfo.h

1315

Similarly to the warning string, I think a name like featureAffectsCodeGen(...) would be more clear in its use.

clang/lib/Basic/Targets/AArch64.cpp

663

Slight nit, but braces aren't needed here.

clang/lib/CodeGen/CodeGenFunction.cpp

2561

Parentheses not needed.

2664

I'm not 100% sure on the differences between assert(..) and LLvm_unreachable(...), but perhaps that would be cleaner than an assertion on false.

2670

Nit on the capital letters for every word. Only "No" requires capitalisation.

clang/lib/Sema/SemaDecl.cpp

10498

Candidate doesn't need capitalisation either.

clang/lib/Sema/SemaDeclAttr.cpp

3521

I think HasCodeImpact would be more explanatory.

[danielkiss]

In D127812#3602688, @aaron.ballman wrote:

In D127812#3602645, @erichkeane wrote:

In D127812#3601476, @danielkiss wrote:

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

I don't think differentiating the uses here is a good idea. I think it would have been a GREAT idea about 10 years ago, but that ship has already sailed once GCC started using it that way however. We should be keeping the current behavior, otherwise we're going to have a horrible mix of target/target_version working inconsistently between platforms.

That largely is my concern as well. The existing behavior of target is just that -- the existing behavior. I think deviating from that existing behavior will be confusing in practice. Adding additional attributes doesn't improve that confusion because users then have to know to decide between two very similar attributes, which means they then need to educate themselves on the differences between them. If we're going to push them towards the documentation to correctly use the attribute anyway, that's basically the same situation they're in today with the confusing dual behavior of target.

We started with the ACLE to be sure the platforms and compilers will implement the same for Arm targets so make the developers life easier with a consistent behaviour on Arm platforms. Users of the attributes anyway need to aware of the architecture differences. Like -mtune is different between Arm/AArch64 and x86.

I have some hope others may see the benefits of the new semantics (e.g. make the "default" optional for target_clones ) will be picked up by other architectures.

[echristo]

In D127812#3616603, @danielkiss wrote:

In D127812#3602688, @aaron.ballman wrote:

In D127812#3602645, @erichkeane wrote:

In D127812#3601476, @danielkiss wrote:

Your understanding is correct. target attribute has two usage model. One is just redefine the to be used codegen options, this is used already widely for Arm and AArch64. The other use of the target attribute is the multi versioning and the rational for the target_version attribute is the easier distinction between the two usage mode, also not to break any code out there by changing the behaviour of an attribute.

I don't think differentiating the uses here is a good idea. I think it would have been a GREAT idea about 10 years ago, but that ship has already sailed once GCC started using it that way however. We should be keeping the current behavior, otherwise we're going to have a horrible mix of target/target_version working inconsistently between platforms.

That largely is my concern as well. The existing behavior of target is just that -- the existing behavior. I think deviating from that existing behavior will be confusing in practice. Adding additional attributes doesn't improve that confusion because users then have to know to decide between two very similar attributes, which means they then need to educate themselves on the differences between them. If we're going to push them towards the documentation to correctly use the attribute anyway, that's basically the same situation they're in today with the confusing dual behavior of target.

We started with the ACLE to be sure the platforms and compilers will implement the same for Arm targets so make the developers life easier with a consistent behaviour on Arm platforms. Users of the attributes anyway need to aware of the architecture differences. Like -mtune is different between Arm/AArch64 and x86.

And that's been frustrating for years too and would have been nice had aarch64 not followed it :)

That said, my desire here is source compatibility between open source compilers - I think that we need at least source compatibility with gcc and then we can go from there.

llvm/lib/Target/AArch64/AArch64.td

74

What is this for?

[ilinpv]

llvm/lib/Target/AArch64/AArch64.td

74

FMV is a target feature which is enabled by default and can be disabled (-mno-fmv ). Accoding to ACLE spec "FMV may be disabled in compile time by a compiler flag. In this case the default version shall be used."

[danielkiss]

Please find the thread on the GCC mailing list here: https://gcc.gnu.org/pipermail/gcc/2022-July/239134.html
Feedback there sounds positive to me.
All received feedback added to this PR.
https://github.com/ARM-software/acle/pull/211

@ilinpv Could you please update the patch accordingly?

(I'm on vacation until September, please expect slow responses.)

[dmgreen]

llvm/include/llvm/Support/AArch64TargetParser.def

188

The compiler already has names for these, used in the existing -march=... options and target(...) attributes. It would be better to not have to invent a new set of names that is slightly different.

It would be good if this could be combined with the table above too. Hopefully it is just an expansion to the values already present.