From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-yb1-xb33.google.com (mail-yb1-xb33.google.com [IPv6:2607:f8b0:4864:20::b33]) by sourceware.org (Postfix) with ESMTPS id 368FE385C313 for ; Fri, 24 Jun 2022 21:26:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 368FE385C313 Received: by mail-yb1-xb33.google.com with SMTP id h187so4362750ybg.0 for ; Fri, 24 Jun 2022 14:26:22 -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=tW30hj1Ix25e3cExGabDZYTWzPca6Ctoltkcakx2EZ0=; b=hIuBgZb2lfR3WOpVZkdpqzXdir2icO+ovAr3QMNGq0gMbJeMN2cOwUyThTA7g1275T yf0u+jgmEtYciWNv3yLC5iSn30O8T/IOruEpYAWVwLV4aZ68PNVdJT8IqLttA3LnKBKx GTSo5UgwMgfg/QqnD/O9jtFR3oXC1vfnleQKJmJBgZcL3cF/87jHOEJBgkE9/IWSgxnF UnGJVD4EHhaoBpU+hKEuzDahcazEiceqDz+X3UiS1otPrDNN3GTEHTYUcl26Lt6IZq7T lTiv0gM4domo5rcgvKFoz7p1W1hyqYMt2vvN1lSYy/cPlscM9PH016EErB8AylXuFLXZ d25g== X-Gm-Message-State: AJIora+RsVoQoic/KJZsJuHlql552bZzBcqHqCpNziTmb65edhuEFCUx YQ4ssRkE6AWHvImO6M6QRNAyvDl99SGukTJQ3t/fAoORPXY= X-Google-Smtp-Source: AGRyM1sNJ4d0xCDRxvpk6m/w4oedxO3XSz9Pd2UE1pe8V5ybPJegcDfhNdrWXKVZz73/9Y1FGoNVMbqPXovwj5sOfTc= X-Received: by 2002:a25:a1a1:0:b0:668:b8e6:8012 with SMTP id a30-20020a25a1a1000000b00668b8e68012mr1195350ybi.526.1656105981638; Fri, 24 Jun 2022 14:26:21 -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: From: Noah Goldstein Date: Fri, 24 Jun 2022 14:26:11 -0700 Message-ID: Subject: Re: [PATCH v2] x86: Fix backwards Prefer_No_VZEROUPPER check in ifunc-evex.h To: "H.J. Lu" Cc: GNU C Library , "Carlos O'Donell" Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-8.4 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 21:26:27 -0000 On Fri, Jun 24, 2022 at 1:33 PM H.J. Lu wrote: > > 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. How about: /* Depending on ISA level some feature checks will default evaluate to true if the MINIMUM_X86_ISA_LEVEL is high enough. The check on a feature will default evaluate to true if: Value <= MINIMUM_X86_ISA_LEVEL. */ ? > > > > > /* 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.