Re: [PATCH] x86-64: Simplify minimum ISA check ifdef conditional with if

Re: [PATCH] x86-64: Simplify minimum ISA check ifdef conditional with if

[H.J. Lu]

On Sun, Mar 3, 2024 at 1:46 PM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Mar 1, 2024 at 4:40 PM Sunil K Pandey <skpgkp2@gmail.com> wrote:
> >
> > Replace minimum ISA check ifdef conditional with if.  Since
> > MINIMUM_X86_ISA_LEVEL and AVX_X86_ISA_LEVEL are compile time constants,
> > compiler will perform constant folding optimization getting same result.
> > ---
> >  sysdeps/x86/cpu-features.c | 19 ++++++++-----------
> >  1 file changed, 8 insertions(+), 11 deletions(-)
> >
> > diff --git a/sysdeps/x86/cpu-features.c b/sysdeps/x86/cpu-features.c
> > index e7c7ece462..4ea373dffa 100644
> > --- a/sysdeps/x86/cpu-features.c
> > +++ b/sysdeps/x86/cpu-features.c
> > @@ -1196,9 +1196,8 @@ no_cpuid:
> >                TUNABLE_CALLBACK (set_x86_shstk));
> >  #endif
> >
> > -#if MINIMUM_X86_ISA_LEVEL < AVX_X86_ISA_LEVEL
> > -  if (GLRO(dl_x86_cpu_features).xsave_state_size != 0)
> > -#endif
> > +  if (MINIMUM_X86_ISA_LEVEL >= AVX_X86_ISA_LEVEL
> > +      || (GLRO(dl_x86_cpu_features).xsave_state_size != 0))
> >      {
> >        if (CPU_FEATURE_USABLE_P (cpu_features, XSAVEC))
> >         {
> > @@ -1219,24 +1218,22 @@ no_cpuid:
> >  #endif
> >         }
> >      }
> > -#if MINIMUM_X86_ISA_LEVEL < AVX_X86_ISA_LEVEL
> >    else
> >      {
> > -# ifdef __x86_64__
> > +#ifdef __x86_64__
> >        GLRO(dl_x86_64_runtime_resolve) = _dl_runtime_resolve_fxsave;
> > -#  ifdef SHARED
> > +# ifdef SHARED
> >        GLRO(dl_x86_tlsdesc_dynamic) = _dl_tlsdesc_dynamic_fxsave;
> > -#  endif
> > -# else
> > -#  ifdef SHARED
> > +# endif
> > +#else
> > +# ifdef SHARED
> >        if (CPU_FEATURE_USABLE_P (cpu_features, FXSR))
> >         GLRO(dl_x86_tlsdesc_dynamic) = _dl_tlsdesc_dynamic_fxsave;
> >        else
> >         GLRO(dl_x86_tlsdesc_dynamic) = _dl_tlsdesc_dynamic_fnsave;
> > -#  endif
> >  # endif
> > -    }
> >  #endif
> > +    }
> >
> >  #ifdef SHARED
> >  # ifdef __x86_64__
> > --
> > 2.43.0
> >
>
> LGTM.
>
> Reviewed-by: H.J. Lu <hjl.tools@gmail.com>
>
> Thanks.
>
> --
> H.J.

Should it also be backported to glibc 2.39 together with

commit befe2d3c4dec8be2cdd01a47132e47bdb7020922
Author: H.J. Lu <hjl.tools@gmail.com>
Date:   Wed Feb 28 09:51:14 2024 -0800

    x86-64: Don't use SSE resolvers for ISA level 3 or above


-- 
H.J.

Re: [PATCH] x86-64: Simplify minimum ISA check ifdef conditional with if

[Florian Weimer]

* H. J. Lu:

> Should it also be backported to glibc 2.39 together with
>
> commit befe2d3c4dec8be2cdd01a47132e47bdb7020922
> Author: H.J. Lu <hjl.tools@gmail.com>
> Date:   Wed Feb 28 09:51:14 2024 -0800
>
>     x86-64: Don't use SSE resolvers for ISA level 3 or above

This is the one that breaks valgrind, isn't it?

I think we'll do downstream backports of some of the x86-64
optimizations, but in the past, there was pushback against doing those
upstream.

Thanks,
Florian

Re: [PATCH] x86-64: Simplify minimum ISA check ifdef conditional with if

[H.J. Lu]

On Sat, Apr 13, 2024 at 10:52 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > Should it also be backported to glibc 2.39 together with
> >
> > commit befe2d3c4dec8be2cdd01a47132e47bdb7020922
> > Author: H.J. Lu <hjl.tools@gmail.com>
> > Date:   Wed Feb 28 09:51:14 2024 -0800
> >
> >     x86-64: Don't use SSE resolvers for ISA level 3 or above
>
> This is the one that breaks valgrind, isn't it?

No.  -march=x86-64-v3 triggers the valgrind error in flibc test.   This
commit fixes the -march=x86-64-v3 build.

commit b6e3898194bbae78910bbe9cd086937014961e45
Author: Sunil K Pandey <skpgkp2@gmail.com>
Date:   Thu Feb 29 17:57:02 2024 -0800

    x86-64: Simplify minimum ISA check ifdef conditional with if

updates this commit.  Backporting the above commit makes the
backported code look similar to upstream.

> I think we'll do downstream backports of some of the x86-64
> optimizations, but in the past, there was pushback against doing those
> upstream.
>
> Thanks,
> Florian
>


-- 
H.J.

Re: [PATCH] x86-64: Simplify minimum ISA check ifdef conditional with if

[Florian Weimer]

* H. J. Lu:

> On Sat, Apr 13, 2024 at 10:52 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * H. J. Lu:
>>
>> > Should it also be backported to glibc 2.39 together with
>> >
>> > commit befe2d3c4dec8be2cdd01a47132e47bdb7020922
>> > Author: H.J. Lu <hjl.tools@gmail.com>
>> > Date:   Wed Feb 28 09:51:14 2024 -0800
>> >
>> >     x86-64: Don't use SSE resolvers for ISA level 3 or above
>>
>> This is the one that breaks valgrind, isn't it?
>
> No.  -march=x86-64-v3 triggers the valgrind error in flibc test.   This
> commit fixes the -march=x86-64-v3 build.
>
> commit b6e3898194bbae78910bbe9cd086937014961e45
> Author: Sunil K Pandey <skpgkp2@gmail.com>
> Date:   Thu Feb 29 17:57:02 2024 -0800
>
>     x86-64: Simplify minimum ISA check ifdef conditional with if
>
> updates this commit.  Backporting the above commit makes the
> backported code look similar to upstream.

yes, I was confused, after all I backported the “x86-64: Don't use SSE
resolvers for ISA level 3 or above” commit and saw the valgrind failure
with it!

I'll run my tests again for the additional backport and push it.

Thanks,
Florian