Re: [RFC] Function Multi Versioning on Arm

["Martin Liška" <mliska@suse.cz>]

On 7/21/22 19:49, Daniel Kiss wrote:
> Hello,
> 
> Thanks for the quick reply, see mine inline.
>> On 2022. Jul 19., at 12:01, Martin Liška <mliska@suse.cz> wrote:
>>
>> On 7/18/22 12:36, Daniel Kiss via Gcc wrote:
>>> Hello,
>>>
>>> We are going to add Function Multiversioning [1] support to Arm architectures.
>>> The specification is made public as beta[2] to ensure toolchain that follows Arm
>>> C Language Extension will implement it in the same way.
>>>
>>> A few tweaks considered to make the developers' life easier.
>>> Since the `target` attribute is used widely on Arm, we would like to introduce a
>>> new attribute `target_version` to avoid confusion and possible deployment
>>> problems. The `target_clones` attribute will be supported too. Also the “default”
>>> version to be made optional.
>>>
>>> We are looking for feedback on the specification (reply, github works too).
>>>
>>> Thanks so much,
>>> Daniel
>>>
>>> [1] https://gcc.gnu.org/onlinedocs/gcc/Function-Multiversioning.html 
>>> [2] https://github.com/ARM-software/acle/blob/main/main/acle.md#function-multi-versioning
>>>
>>
>> Hello.
>>
>> Thanks for working on the feature, it will be nice to cover the gap in between x86_64 and powerpc,
>> which implement the FMV feature.
>>
>> As the person who's been involved with the current MVC code in the GCC, I have a few comments/questions
>> about it:
>>
>> 1) both i386 and Powerpc also allow specifying an equivalent to -march (like `arch=bdver2`),
>>   in Arm case it seems to me only individual features are considered
> Arm architecture version is not definite enough in this case because
> certain features are optional on a given versions and may back ported to older versions.
> Implementation name of a core also could be misleading in most of the cases. And too many out there if
> all implementation is considered not just Arm’s Cortex cores.
> Also the kernel support varies regardless the actual hardware, features can be disabled by the firmware/OS.
> I think developers target a given feature instead of a given uarch most cases.

Sure, that makes fully sense to me!

> 
>>
>> 2) about 'target_version' attribute - I like the idea as one can have a completely independent
>>   function implementation optimized for an ISA;
>>   note it's very close to 'target' attribute (supported in C++), where one needs to provide
>>   a resolver and have the pretty same functionality (see e.g. gcc/testsuite/g++.target/i386/mv1.C).
>>   However, the feature does not work in C and you will have the very same problem with target_version
>>   attribute (multiple functions with the same declaration):
>>
>>  mv1.c:76:1: error: redefinition of ‘foo’
>>     76 | foo ()
>>        | ^~~
> In our clang implementation\prototype such a use case is supported. The goal was to able to write like this in C
> /* existing code*/
> extern int foo();
> int foo(){}
> /* addition */
> #ifdef __ARM_FEATURE_FUNCTION_MULTI_VERSIONING
> __attribute__((target_version(“...")))
> int foo(){}
> #endif

I see, so then it's going to require a more work regarding the C front-end. Maybe we should enable the same way the "target"
attribute for C.

> 
> so an existing codebase can be extended without breaking it even for old compilers, without heavy checks for attribute support.

Yep.

> 
>> 3) If you will implement 'target_version' attribute, I would like to see it available also for the
>>   existing targets supporting MVC
> Yes, this is the plan if other target maintainers will accept it.
> IMHO all semantical differences would work for all targets.

Sure!

>>
>> 4) A small note about the mangling, the existing i386 naming scheme looks like:
>>
>> ...
>> _Z3foov.avx2_ssse3
>> ...
>>
>> 5) Can you please define how will you evaluate priorities for a situation where multiple features
>>   are used (e.g. dotprod+sm)?
>>
>>   Note we face the very same problem on i386, where it's very hard to specify a priority
>>   for the family of avx512 features. That said, an optional priority specifier might be possible.
> ACLE provides a table of priorities for given feature and a simple algorithm how to choose.
> 
> Version where the most features are requested will be picked,

Ok!

> then the one with the highest priority.
> in case of (dotprod+sm, sve) set the dotprod+sm will be selected just because it is more specified, even
> sve has higher priority.
> 
> We considered the other of the attributes in the source, but that might be quite problematic to preserve during
> compilation.

We can start with that and add priorities later if really needed.

> 
> A new attribute or variant that provides priority could work too, just so far the newer feature usually a better
> choice, and those got higher priority.
> 
>>
>> 6) Note that as opposed to i385 and Powerpc, we don't allow a combination of ISA flags for target_clone
>>   attribute (like sse2+avx512f).
> Noted, I think in case of Arm it may make sense to support it.
>>
>> 7) FMV may be disabled in compile time by a compiler flag. In this case the default version shall be used.
>>
>>   I would like to see the functionality also target agnostic.
> Sure, I agree.  the proposed flag is -mno-fmv (-mfmv default on). 
> Also maybe the feature indication define __ARM_FEATURE_FUNCTION_MULTI_VERSIONING could be just
> __FEATURE_FUNCTION_MULTI_VERSIONING?

I would take a name inspiration from:
https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html

so what about something like __HAVE_FUNCTION_MULTI_VERSIONING ?

Cheers,
Martin

> 
>>
>> Anyway, looking forward to the Arm implementation.
>> Hope the comments are constructive.
> Thanks, help me a lot.
> 
>>
>> Cheers,
>> Martin
>