From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 80C0E386CE4B for ; Fri, 24 Jun 2022 20:33:20 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 80C0E386CE4B Received: by mail-pg1-x52f.google.com with SMTP id g186so3430896pgc.1 for ; Fri, 24 Jun 2022 13:33:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=khxofBeb1Q92bO+MxlqXNgVuMUrNbg7x2ZK/Oi41kp0=; b=pGsKr0BciN8PIKB2lPiL2rZExcxMP/aLus4mLqPrMwyAZGHjgfpt7JwB6m/XM0m6AW 8uvczQbd9g9bCvLTgnJUvpX+l8+o6udxnwJNMvAFPXnqQOLj4e2AahDdJeakY9Dc2MM1 wfu7YJRbAkIKcsrn8gzRa13lPF+SYdlZrTK4PpzqbzEt22sJrnZMuhWzo2oBMRo2Yyr7 xkSDTNQxnERLYqUnZPNlMKY/D47sT9llcW4rEqrKsttHFIWafdECiuJsx/Fr9NCpbiMq gL2JnpgOLY56SxUncVpuaeaqKaKcEXEXCXBl3ZSZm/5czcRC8/++opAbxGyOviUlJzRn xP4Q== X-Gm-Message-State: AJIora8o/5AWgNTbDV+r2AJhz4q1gzDOYyRdUgbuL8m8zcA1gTWafAZ8 ciwcvzdl7hBV8qmFabsHmxP4cVlzEALv7ONGWpA69seGZCw= X-Google-Smtp-Source: AGRyM1sv9PXcGKB6pzGT1aifw1PevuKb0Ox8jkTQ8+3CcvLjBbV/zfoR1Rzr7DC5LlcCjTPzMDcqsrwnAlA4MoziDMw= X-Received: by 2002:a63:3c14:0:b0:40c:a228:c3c3 with SMTP id j20-20020a633c14000000b0040ca228c3c3mr601362pga.256.1656102799317; Fri, 24 Jun 2022 13:33:19 -0700 (PDT) MIME-Version: 1.0 References: <20220624063653.2126416-3-goldstein.w.n@gmail.com> <20220624201036.3740866-1-goldstein.w.n@gmail.com> In-Reply-To: <20220624201036.3740866-1-goldstein.w.n@gmail.com> From: "H.J. Lu" Date: Fri, 24 Jun 2022 13:32:43 -0700 Message-ID: Subject: Re: [PATCH v2] x86: Fix backwards Prefer_No_VZEROUPPER check in ifunc-evex.h To: Noah Goldstein Cc: GNU C Library , "Carlos O'Donell" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-3025.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libc-alpha@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libc-alpha mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Jun 2022 20:33:24 -0000 On Fri, Jun 24, 2022 at 1:10 PM Noah Goldstein wrote: > > Add third argument to X86_ISA_CPU_FEATURES_ARCH_P macro so the runtime > CPU_FEATURES_ARCH_P check can be inverted if the > MINIMUM_X86_ISA_LEVEL is not high enough to constantly evaluate > the check. > > Use this new macro to correct the backwards check in ifunc-evex.h > --- > sysdeps/x86/isa-ifunc-macros.h | 29 +++++++++++++++++++++------ > sysdeps/x86/isa-level.h | 26 +++++++++--------------- > sysdeps/x86_64/multiarch/ifunc-evex.h | 4 ++-- > 3 files changed, 35 insertions(+), 24 deletions(-) > > diff --git a/sysdeps/x86/isa-ifunc-macros.h b/sysdeps/x86/isa-ifunc-macros.h > index ba6826d518..a3c98c841c 100644 > --- a/sysdeps/x86/isa-ifunc-macros.h > +++ b/sysdeps/x86/isa-ifunc-macros.h > @@ -56,15 +56,32 @@ > # define X86_IFUNC_IMPL_ADD_V1(...) > #endif > > -#define X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED(name) \ > - ((name##_X86_ISA_LEVEL) <= MINIMUM_X86_ISA_LEVEL) > +/* Both X86_ISA_CPU_FEATURE_USABLE_P and X86_ISA_CPU_FEATURES_ARCH_P > + should only be used to check if a condition is true. I.e: > + > + if (X86_ISA_CPU_FEATURE{S}_{USABLE|ARCH}_P (...)) // Good > + if (!X86_ISA_CPU_FEATURE{S}_{USABLE|ARCH}_P (...)) // Bad If (X86_ISA_CPU_FEATURE{S}_{USABLE|ARCH}_P (...)) works, if (!X86_ISA_CPU_FEATURE{S}_{USABLE|ARCH}_P (...)) should also work. > + > + There should be no need for inverting USABLE_P checks, but there is > + often need for inverting ARCH_P checks. If you want to get the not > + of an ARCH_P feature do: > + > + if (X86_ISA_CPU_FEATURES_ARCH_P (..., !)) // Good > + */ > + > > #define X86_ISA_CPU_FEATURE_USABLE_P(ptr, name) \ > - (X86_ISA_CPU_FEATURE_CONST_CHECK_ENABLED (name) \ > + (((name##_X86_ISA_LEVEL) <= MINIMUM_X86_ISA_LEVEL) \ > || 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)) > + > +/* When using X86_ISA_CPU_FEATURES_ARCH_P a third argument must be > + provided to optionally invert the runtime CPU_FEATURES_ARCH_P > + check. This is so we can consistently constant-evaluate conditions > + using Feature_X86_ISA_LEVEL <= MINIMUM_X86_ISA_LEVEL. */ > +#define X86_ISA_CPU_FEATURES_ARCH_P(ptr, name, not) \ > + (((name##_X86_ISA_LEVEL) <= MINIMUM_X86_ISA_LEVEL) \ > + || not CPU_FEATURES_ARCH_P (ptr, name)) > + > > #endif > diff --git a/sysdeps/x86/isa-level.h b/sysdeps/x86/isa-level.h > index 7cae11c228..bad9aba099 100644 > --- a/sysdeps/x86/isa-level.h > +++ b/sysdeps/x86/isa-level.h > @@ -65,12 +65,8 @@ > (__X86_ISA_V1 + __X86_ISA_V2 + __X86_ISA_V3 + __X86_ISA_V4) > > > -/* > - * CPU Features that are hard coded as enabled depending on ISA build > - * level. > - * - Values > 0 features are always ENABLED if: > - * Value >= MINIMUM_X86_ISA_LEVEL > - */ > +/* CPU Features that are default set depending on ISA build level. > + Feature is assumed set if: Value <= MINIMUM_X86_ISA_LEVEL. */ This isn't accurate for Prefer_No_VZEROUPPER_X86_ISA_LEVEL. I think this should be removed. Each feature needs a comment to describe the default. > > /* ISA level >= 4 guaranteed includes. */ > @@ -81,18 +77,16 @@ > #define AVX2_X86_ISA_LEVEL 3 > #define BMI2_X86_ISA_LEVEL 3 > > -/* > - * NB: This may not be fully assumable for ISA level >= 3. From > - * looking over the architectures supported in cpu-features.h the > - * following CPUs may have an issue with this being default set: > - * - AMD Excavator > - */ > +/* 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 > > -/* > - * KNL (the only cpu that sets this supported in cpu-features.h) > - * builds with ISA V1 so this shouldn't harm any architectures. > - */ > +/* 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 > > #define ISA_SHOULD_BUILD(isa_build_level) \ > diff --git a/sysdeps/x86_64/multiarch/ifunc-evex.h b/sysdeps/x86_64/multiarch/ifunc-evex.h > index 856c6261f8..310cfd269f 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); > } > > -- > 2.34.1 > -- H.J.