[PATCH v2 1/2] x86: Add macro for NOT of a cpu arch feature and improve comments

[Noah Goldstein]

On Fri, Jun 24, 2022 at 11:19 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Fri, Jun 24, 2022 at 9:41 AM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
> >
> > Some ARCH_P features such as Prefer_No_VZEROUPPER used to disable
> > implementations if true as opposed fo the majority of features
> > such as AVX2 which are used to enabled features.
> >
> > Different ISA build levels want override certain disabling
> > features. For example ISA build level >= 3 should ignore
> > Prefer_No_VZEROUPPER which means converting the check to
> > false (as opposed to true for a feature like AVX2).
> > ---
> >  sysdeps/x86/isa-ifunc-macros.h | 4 ++++
> >  sysdeps/x86/isa-level.h        | 7 ++++---
> >  2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/sysdeps/x86/isa-ifunc-macros.h b/sysdeps/x86/isa-ifunc-macros.h
> > index ba6826d518..e229e612a4 100644
> > --- a/sysdeps/x86/isa-ifunc-macros.h
> > +++ b/sysdeps/x86/isa-ifunc-macros.h
> > @@ -67,4 +67,8 @@
> >    (X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name)                      \
> >     || CPU_FEATURES_ARCH_P (ptr, name))
> >
> > +#define X86_ISA_CPU_FEATURES_ARCH_P_NOT(ptr, name)                            \
> > +  (!X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name)                           \
> > +     && !CPU_FEATURES_ARCH_P (ptr, name))
> > +
>
> This is incorrect.  I prefer
>
> #define X86_ISA_CPU_FEATURES_ARCH_P(ptr, name, not)                    \
>   (X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name)                      \
>    || not (CPU_FEATURES_ARCH_P (ptr, name)))
>
> to force a choice.

Done in V2.
>
> >  #endif
> > diff --git a/sysdeps/x86/isa-level.h b/sysdeps/x86/isa-level.h
> > index 7cae11c228..e1a30ed83e 100644
> > --- a/sysdeps/x86/isa-level.h
> > +++ b/sysdeps/x86/isa-level.h
> > @@ -66,10 +66,10 @@
> >
> >
> >  /*
> > - * CPU Features that are hard coded as enabled depending on ISA build
> > - *   level.
> > + * CPU Features that are hard coded as enabled/disabled depending on
> > + * ISA build level.
> >   *    - Values > 0 features are always ENABLED if:
> > - *          Value >= MINIMUM_X86_ISA_LEVEL
> > + *          Value <= MINIMUM_X86_ISA_LEVEL
> >   */
>
> This isn't accurate for Prefer_No_VZEROUPPER.  Also there should be
> no leading * in comments.  How about
>
> /* ISA and architecture features that depend on ISA level.  */
>
> /* ISA level >= 4 guaranteed includes.  */
> #define AVX512VL_X86_ISA_LEVEL 4
> #define AVX512BW_X86_ISA_LEVEL 4
>
> /* ISA level >= 3 guaranteed includes.  */
> #define AVX2_X86_ISA_LEVEL 3
> #define BMI2_X86_ISA_LEVEL 3
>
> /* NB: This feature is enabled when ISA level >= 3, which was disabled
>    for the following CPUs:
>    - AMD Excavator
>    when ISA level < 3.  */
> #define AVX_Fast_Unaligned_Load_X86_ISA_LEVEL 3
>
> /* NB: This feature is disabled when ISA level >= 3, which was enabled
>    for the following CPUs:
>    - Intel KNL
>    when ISA level < 3.  */
> #define Prefer_No_VZEROUPPER_X86_ISA_LEVEL 3
>

Cleaned up comments and remove leading "*" in comments.
> >
> > @@ -92,6 +92,7 @@
> >  /*
> >   * KNL (the only cpu that sets this supported in cpu-features.h)
> >   * builds with ISA V1 so this shouldn't harm any architectures.
> > + * NB: Only use this feature with the ARCH_P_NOT macro.
> >   */
> >  #define Prefer_No_VZEROUPPER_X86_ISA_LEVEL 3
> >
> > --
> > 2.34.1
> >
>
>
>
> --
> H.J.