[PATCH] D127812: [AArch64] Function multiversioning support added.

[Erich Keane via Phabricator via llvm-commits]

erichkeane added a comment.

In D127812#3601476 <https://reviews.llvm.org/D127812#3601476>, @danielkiss wrote:

> In D127812#3599530 <https://reviews.llvm.org/D127812#3599530>, @aaron.ballman wrote:
>
>> In D127812#3587223 <https://reviews.llvm.org/D127812#3587223>, @ilinpv wrote:
>>
>>> In D127812#3585249 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127812