Re: [RFC] Function Multi Versioning on Arm

[Daniel Kiss <Daniel.Kiss@arm.com>]

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.

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

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

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

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?

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

> 
> Cheers,
> Martin