Page MenuHomePhabricator

[AArch64] Function multiversioning support added.
Needs RevisionPublic

Authored by ilinpv on Jun 14 2022, 4:53 PM.

Details

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!

Diff Detail

Event Timeline

ilinpv created this revision.Jun 14 2022, 4:53 PM
Herald added a project: Restricted Project. · View Herald Transcript
ilinpv requested review of this revision.Jun 14 2022, 4:53 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald TranscriptJun 14 2022, 4:53 PM
Herald added subscribers: llvm-commits, Restricted Project, cfe-commits, MaskRay. · View Herald Transcript
tahonermann added a subscriber: tahonermann.
MaskRay added inline comments.Jun 14 2022, 9:34 PM
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.

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 added a comment.EditedJun 15 2022, 3:17 PM

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 added inline comments.Jun 15 2022, 4:32 PM
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 added inline comments.Jun 15 2022, 4:49 PM
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 updated this revision to Diff 438130.Jun 18 2022, 8:41 AM
ilinpv added subscribers: sdesmalen, DylanFleming-arm.

Addressing comments, stylistic corrections.

ilinpv marked 3 inline comments as done.Jun 18 2022, 8:44 AM
xbolva00 added inline comments.
clang/docs/ClangCommandLineReference.rst
1086 ↗(On Diff #436971)

But it needs to be manually autogenerated..

ilinpv added inline comments.Jun 20 2022, 4:16 AM
clang/docs/ClangCommandLineReference.rst
1086 ↗(On Diff #436971)

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.

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

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 added inline comments.Jun 28 2022, 3:18 AM
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.

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.

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 added inline comments.Jul 14 2022, 4:37 AM
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 requested changes to this revision.Fri, Jul 29, 7:28 AM

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

This revision now requires changes to proceed.Fri, Jul 29, 7:28 AM
Matt added a subscriber: Matt.Tue, Aug 2, 10:21 PM
dmgreen added a subscriber: dmgreen.
dmgreen added inline comments.
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.