[v2,1/2] x86: Add macro for NOT of a cpu arch feature and improve comments
Checks
| Context |
Check |
Description |
| dj/TryBot-apply_patch |
success
|
Patch applied to master at the time it was sent
|
Commit Message
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
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.
> #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
>
> @@ -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
>
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.
@@ -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
@@ -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