Re: [PATCH] x86_64: Exclude SSE, AVX and FMA4 variants in libm multiarch

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

On 20/02/24 16:56, Adhemerval Zanella Netto wrote:
> 
> 
> On 20/02/24 16:10, H.J. Lu wrote:
>> On Tue, Feb 20, 2024 at 11:02 AM Adhemerval Zanella Netto
>> <adhemerval.zanella@linaro.org> wrote:
>>>
>>>
>>>
>>> On 20/02/24 15:54, H.J. Lu wrote:
>>>> On Tue, Feb 20, 2024 at 10:48 AM Adhemerval Zanella Netto
>>>> <adhemerval.zanella@linaro.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 20/02/24 15:36, H.J. Lu wrote:
>>>>>> On Tue, Feb 20, 2024 at 10:32 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>>>>>>
>>>>>>> On Tue, Feb 20, 2024 at 6:28 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Tue, Feb 20, 2024 at 10:19 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>>>>>>>>
>>>>>>>>> On Tue, Feb 20, 2024 at 6:14 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>>>
>>>>>>>>>> On Tue, Feb 20, 2024 at 10:07 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Tue, Feb 20, 2024 at 6:05 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Tue, Feb 20, 2024 at 9:56 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Tue, Feb 20, 2024 at 5:51 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On Tue, Feb 20, 2024 at 9:34 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On Tue, Feb 20, 2024 at 4:58 PM Sunil K Pandey <skpgkp2@gmail.com> wrote:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> When glibc is built with FMA and AVX2 enabled by default, the resulting
>>>>>>>>>>>>>>>> glibc binaries won't run on SSE or FMA4 processors.  Exclude SSE, AVX and
>>>>>>>>>>>>>>>> FMA4 variants in libm multiarch when both FMA and AVX2 are enabled by
>>>>>>>>>>>>>>>> default.  Disallow glibc build with only AVX2 or FMA enabled as all AVX2
>>>>>>>>>>>>>>>> processors, including VMs, should also support FMA and vice versa.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> When glibc is built with SSE4.1 enabled by default, only keep SSE4.1
>>>>>>>>>>>>>>>> variant.
>>>>>>>>>>>>>>> Not avx2 + FMA as well?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Correct. Logic is as follows
>>>>>>>>>>>>>> If (build with AVX2+FMA): Keep AVX2+FMA variants only.
>>>>>>>>>>>>>> else if (build with SSE4.1): Keep SSE4.1 variants only.
>>>>>>>>>>>>> What if someone builds with sse4.1 as a minimum but then
>>>>>>>>>>>>> runs on avx2+ machines?
>>>>>>>>>>>>
>>>>>>>>>>>> Only SSE4.1 variant will be used in this case.   Both SSE4.1
>>>>>>>>>>>> and AVX versions only have a single instruction.  This matches
>>>>>>>>>>>> the compiler builtin function of SS4.1 and AVX.
>>>>>>>>>>>
>>>>>>>>>>> if they are all the same, whats the rationale for having an
>>>>>>>>>>> avx version at all?
>>>>>>>>>>
>>>>>>>>>> They aren't the same.  For ceil, it is
>>>>>>>>>>
>>>>>>>>>> roundsd $10, %xmm0, %xmm0
>>>>>>>>>> ret
>>>>>>>>>>
>>>>>>>>>> vs
>>>>>>>>>>
>>>>>>>>>> vroundsd $10, %xmm0, %xmm0, %xmm0
>>>>>>>>>> ret
>>>>>>>>>>
>>>>>>>>>> You get the same things with
>>>>>>>>>>
>>>>>>>>>> return __builtin_ceil (x);
>>>>>>>>>
>>>>>>>>> I mean if they are equal quality sse4.1 / avx,
>>>>>>>>> why not just remove the avx impls are using sse4.1 impls
>>>>>>>>> on avx targets?
>>>>>>>>
>>>>>>>> If glibc is compiled with AVX, we should use the AVX version if
>>>>>>>> appropriate.   Since the minimum GCC for glibc build can't inline
>>>>>>>>  __builtin_ceil, we inline  __builtin_ceil by hand.
>>>>>>> if compiled with avx, but for generic target do we need to hold
>>>>>>> onto avx versions for any reason?
>>>>>>
>>>>>> I don't understand what you were asking.   This patch leads to the same
>>>>>> assembly code generated from
>>>>>>
>>>>>> double
>>>>>> __ceil (double x)
>>>>>> {
>>>>>>   return __builtin_ceil (x);
>>>>>> }
>>>>>
>>>>> Wouldn't make sense to follow the already define x86_64 ABI versions and
>>>>> provided the ifunc variants based on the ABI uses?
>>>>
>>>> There are no conflicts here.  For these math functions, ISA level 2 == SSE4.1
>>>> and ISA level 3 == AVX2 + FMA.   If glibc is built with ISA level N, this patch
>>>> will exclude ISA level N-1 or older variants in IFUNC selection.
>>>>
>>>
>>> I mean, why not use the MINIMUM_X86_ISA_LEVEL to define whether to provide/build
>>> the variants instead of adding two new configure checks?
>>
>> One issue is that the minimum GCC (GCC 6?) doesn't support -march=x86-64-vN.
>> Another reason is that these math functions don't need the full ISA
>> level instructions.
>>
> 
> But afaiu gcc support for -march=x86_64-vN does not really matter, isa-level.h
> will define MINIMUM_X86_ISA_LEVEL based compiler preprocessor that should be
> compatible with GCC 6.
> 
> So instead of checking for HAVE_X86_AVX2_FMA, it would be for
> MINIMUM_X86_ISA_LEVEL >= 3. It does not work for C files that explicit uses
> -march, but I don't that is the case here.

I understand that math support is not really tied to x86_64-vX, but I also think
that there is not strong reason to also use the same logic libc.so is already
using on ifunc variant to provide the ifunc variants on libm.so.

It simplifies the testing and minimize the build permutation, since I know that
checking for -march=x86_64-v{1,2,3,4} should be suffice instead of adding
-mfma, etc.

So I also think that instead of libc_cv_have_x86_avx2_fma/libc_cv_have_x86_sse4_1,
it should check for libc_cv_x86_64_vN (similar to what isa-level.h does).  Also,
I don't see much point in adding support for 32 bits.