diff mbox series

[v1,3/7] x86: Add macro for NOT of a cpu arch feature and improve comments

Message ID 20220624063653.2126416-3-goldstein.w.n@gmail.com
State New
Headers show
Series [v1,1/7] x86: Align entry for memrchr to 64-bytes. | expand

Commit Message

Noah Goldstein June 24, 2022, 6:36 a.m. UTC 
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(-)

Comments

H.J. Lu June 24, 2022, 2:32 p.m. UTC | #1 
On Thu, Jun 23, 2022 at 11:37 PM 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))
> +
>  #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
>   */
>
>
> @@ -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
>

This is a bug fix.  Please make it a separate patch set.   Please also
send individual patches,
instead of a set, if there are no dependencies between patches.

Thanks.
H.J. Lu June 24, 2022, 2:49 p.m. UTC | #2 
On Fri, Jun 24, 2022 at 7:32 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Jun 23, 2022 at 11:37 PM 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))
> > +
> >  #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
> >   */
> >
> >
> > @@ -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
> >
>
> This is a bug fix.  Please make it a separate patch set.   Please also
> send individual patches,
> instead of a set, if there are no dependencies between patches.
>

How about something like this?

diff --git a/sysdeps/x86/isa-ifunc-macros.h b/sysdeps/x86/isa-ifunc-macros.h
index ba6826d518..108d9cf023 100644
--- a/sysdeps/x86/isa-ifunc-macros.h
+++ b/sysdeps/x86/isa-ifunc-macros.h
@@ -63,8 +63,8 @@
   (X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name)                      \
    || CPU_FEATURE_USABLE_P (ptr, name))

-#define X86_ISA_CPU_FEATURES_ARCH_P(ptr, name)                         \
-  (X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name)                      \
-   || CPU_FEATURES_ARCH_P (ptr, name))
+#define X86_ISA_CPU_FEATURES_ARCH_P(ptr, name, not)                    \
+  (not (X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name)                 \
+       || CPU_FEATURES_ARCH_P (ptr, name)))

 #endif
diff --git a/sysdeps/x86_64/multiarch/ifunc-evex.h
b/sysdeps/x86_64/multiarch/ifunc-evex.h
index 856c6261f8..2d59e022f0 100644
--- a/sysdeps/x86_64/multiarch/ifunc-evex.h
+++ b/sysdeps/x86_64/multiarch/ifunc-evex.h
@@ -37,7 +37,7 @@ IFUNC_SELECTOR (void)
   if (X86_ISA_CPU_FEATURE_USABLE_P (cpu_features, AVX2)
       && X86_ISA_CPU_FEATURE_USABLE_P (cpu_features, BMI2)
       && X86_ISA_CPU_FEATURES_ARCH_P (cpu_features,
-                 AVX_Fast_Unaligned_Load))
+                 AVX_Fast_Unaligned_Load,))
     {
       if (X86_ISA_CPU_FEATURE_USABLE_P (cpu_features, AVX512VL)
     && X86_ISA_CPU_FEATURE_USABLE_P (cpu_features, AVX512BW))
@@ -52,7 +52,7 @@ IFUNC_SELECTOR (void)
   return OPTIMIZE (avx2_rtm);

       if (X86_ISA_CPU_FEATURES_ARCH_P (cpu_features,
-                  Prefer_No_VZEROUPPER))
+                  Prefer_No_VZEROUPPER, !))
   return OPTIMIZE (avx2);
     }
Noah Goldstein June 24, 2022, 4:43 p.m. UTC | #3 
On Fri, Jun 24, 2022 at 7:33 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Jun 23, 2022 at 11:37 PM 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))
> > +
> >  #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
> >   */
> >
> >
> > @@ -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
> >
>
> This is a bug fix.  Please make it a separate patch set.   Please also
> send individual patches,
> instead of a set, if there are no dependencies between patches.

Split patches. This and the change to ifunc-evex in series. Rest standalone.
>
> Thanks.
>
> --
> H.J.
diff mbox series

Patch

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))
+
 #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
  */
 
 
@@ -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