On 2022. Jul 22., at 10:12, Martin Liška <mli...@suse.cz<mailto:mli...@suse.cz>> wrote:
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 <mli...@suse.cz<mailto:mli...@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. IMHO it would make sense if the “default" attribute becomes optional for “target”. 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 ? Right, __FEATURE… is from the ACLE style so __HAVE_FUNCTION_MULTI_VERSIONING is better for target agnostic feature. I’ll will update ACLE with all feedback that I got. Cheers, Martin Anyway, looking forward to the Arm implementation. Hope the comments are constructive. Thanks, help me a lot. Cheers, Martin
