| Message ID | 20221002123424.3079805-1-aurelien@aurel32.net |
|---|---|
| Headers |
Return-Path: <libc-alpha-bounces+patchwork=sourceware.org@sourceware.org> X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B41473851C36 for <patchwork@sourceware.org>; Sun, 2 Oct 2022 12:34:52 +0000 (GMT) X-Original-To: libc-alpha@sourceware.org Delivered-To: libc-alpha@sourceware.org Received: from hall.aurel32.net (hall.aurel32.net [IPv6:2001:bc8:30d7:100::1]) by sourceware.org (Postfix) with ESMTPS id AB5713858407 for <libc-alpha@sourceware.org>; Sun, 2 Oct 2022 12:34:30 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AB5713858407 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=aurel32.net Authentication-Results: sourceware.org; spf=none smtp.mailfrom=aurel32.net DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=aurel32.net ; s=202004.hall; h=Content-Transfer-Encoding:MIME-Version:Message-Id:Date: Subject:Cc:To:From:Content-Type:From:Reply-To:Subject:Content-ID: Content-Description:In-Reply-To:References:X-Debbugs-Cc; bh=JBsJxnUN3IkbzD9SnZhtKic8HvbWHO7c7qd9tWk1oSw=; b=wiF6ra+GyLwOzq1dKTsPzZ27yG 41ZpHWBRvr0XzvhP8TGqMmtrJ8ODZNkBBcNC4RgwBwMKC9aQuliRnk2owuVfNhGtcZ9B62aE54nHr Yn0x7dsC++HfXon9PhfEhEiSVLYqWLD/ZVDyUjyJ9e7MZQ0HKOoJDy0eZ0mC+RhXc9Ksotzg7aAtE 4UHPBeDwj9+cPiBwFJtkVOX41Q/XFpGnbRxwkkxQHSCcceudjKa1r7EzMXeLChyH8Wqsz3rZ9XTS9 eVqToyIx8D5J3lReeRiPKav/PWRQqAph8Z5GvSX/rzF4eKKQU1FXKhQmlB2aWorlrM9Yql8mvnk5P VtfJzr0w==; Received: from [2a01:e34:ec5d:a741:8a4c:7c4e:dc4c:1787] (helo=ohm.rr44.fr) by hall.aurel32.net with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from <aurelien@aurel32.net>) id 1oeyAu-00FLU8-Hd; Sun, 02 Oct 2022 14:34:28 +0200 Received: from aurel32 by ohm.rr44.fr with local (Exim 4.96) (envelope-from <aurelien@aurel32.net>) id 1oeyAt-00CvFu-2I; Sun, 02 Oct 2022 14:34:27 +0200 From: Aurelien Jarno <aurelien@aurel32.net> To: libc-alpha@sourceware.org Subject: [PATCH v2 0/6] x86: Fix AVX2 string functions requiring BMI1, BMI2 or LZCNT (BZ #29611) Date: Sun, 2 Oct 2022 14:34:18 +0200 Message-Id: <20221002123424.3079805-1-aurelien@aurel32.net> X-Mailer: git-send-email 2.35.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-7.2 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, SPF_HELO_PASS, SPF_NONE, TXREP 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 <libc-alpha.sourceware.org> List-Unsubscribe: <https://sourceware.org/mailman/options/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=unsubscribe> List-Archive: <https://sourceware.org/pipermail/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-request@sourceware.org?subject=help> List-Subscribe: <https://sourceware.org/mailman/listinfo/libc-alpha>, <mailto:libc-alpha-request@sourceware.org?subject=subscribe> Cc: Aurelien Jarno <aurelien@aurel32.net> Errors-To: libc-alpha-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libc-alpha" <libc-alpha-bounces+patchwork=sourceware.org@sourceware.org> |
| Series |
x86: Fix AVX2 string functions requiring BMI1, BMI2 or LZCNT (BZ #29611)
|
|
Message
Aurelien Jarno
Oct. 2, 2022, 12:34 p.m. UTC
Some early Intel Haswell CPU have AVX2 instructions, but do not have BMI1 and BMI2 instructions. Some AVX2 string functions only check for AVX2, but use BMI1, BMI2 or LZCNT instructions. This patchset tries to fix that. While most fixes only change ifunc-impl-list.c, and thus only concerns the testsuite, the strn(case)cmp is a real issue affecting early Intel Haswell CPU, reported to affect Debian Sid and Fedora Rawhide. On the other hand, the check for LZCNT in memrchr is purely for correctness, I am not aware of a CPU implementing AVX2 without LZCNT. This has been tested by remplacing all BMI1 and BMI2 instructions in the source code by the "ud2" instruction and disabling the BMI1, BMI2 feature detection, and running the testsuite. Resolves: BZ #29611 Change v1 -> v2: - Better scan for BMI2 instructions (shlx and shrx) and BMI1 instructions (blsmsk) instructions following the feedback from Noah Goldstein Aurelien Jarno (6): x86: include BMI1 and BMI2 in x86-64-v3 level x86-64: Require BMI2 for AVX2 str*cmp and wcs(n)cmp implementations x86-64: Require BMI2 for AVX2 (raw|w)memchr implementations x86-64: Require LZCNT for AVX2 memrchr implementation x86-64: Require BMI1/BMI2 for AVX2 strrchr and wcsrchr implementations x86-64: Require BMI2 for AVX2 memrchr implementation sysdeps/x86/get-isa-level.h | 2 + sysdeps/x86/isa-level.h | 2 + sysdeps/x86_64/multiarch/ifunc-avx2.h | 2 + sysdeps/x86_64/multiarch/ifunc-impl-list.c | 86 ++++++++++++++++----- sysdeps/x86_64/multiarch/ifunc-strcasecmp.h | 1 + sysdeps/x86_64/multiarch/strcmp.c | 4 +- sysdeps/x86_64/multiarch/strncmp.c | 4 +- 7 files changed, 76 insertions(+), 25 deletions(-)
Comments
On Sun, Oct 2, 2022 at 5:34 AM Aurelien Jarno <aurelien@aurel32.net> wrote: > > Some early Intel Haswell CPU have AVX2 instructions, but do not have > BMI1 and BMI2 instructions. Some AVX2 string functions only check for > AVX2, but use BMI1, BMI2 or LZCNT instructions. This patchset tries to > fix that. > > While most fixes only change ifunc-impl-list.c, and thus only concerns > the testsuite, the strn(case)cmp is a real issue affecting early Intel str(case)cmp as well, correct? > Haswell CPU, reported to affect Debian Sid and Fedora Rawhide. > > On the other hand, the check for LZCNT in memrchr is purely for > correctness, I am not aware of a CPU implementing AVX2 without LZCNT. > > This has been tested by remplacing all BMI1 and BMI2 instructions in the > source code by the "ud2" instruction and disabling the BMI1, BMI2 > feature detection, and running the testsuite. > > Resolves: BZ #29611 > > Change v1 -> v2: > - Better scan for BMI2 instructions (shlx and shrx) and BMI1 > instructions (blsmsk) instructions following the feedback from Noah > Goldstein > > Aurelien Jarno (6): > x86: include BMI1 and BMI2 in x86-64-v3 level > x86-64: Require BMI2 for AVX2 str*cmp and wcs(n)cmp implementations > x86-64: Require BMI2 for AVX2 (raw|w)memchr implementations > x86-64: Require LZCNT for AVX2 memrchr implementation > x86-64: Require BMI1/BMI2 for AVX2 strrchr and wcsrchr implementations > x86-64: Require BMI2 for AVX2 memrchr implementation > > sysdeps/x86/get-isa-level.h | 2 + > sysdeps/x86/isa-level.h | 2 + > sysdeps/x86_64/multiarch/ifunc-avx2.h | 2 + > sysdeps/x86_64/multiarch/ifunc-impl-list.c | 86 ++++++++++++++++----- > sysdeps/x86_64/multiarch/ifunc-strcasecmp.h | 1 + > sysdeps/x86_64/multiarch/strcmp.c | 4 +- > sysdeps/x86_64/multiarch/strncmp.c | 4 +- > 7 files changed, 76 insertions(+), 25 deletions(-) > > -- > 2.35.1 >
On 2022-10-02 09:21, Noah Goldstein wrote: > On Sun, Oct 2, 2022 at 5:34 AM Aurelien Jarno <aurelien@aurel32.net> wrote: > > > > Some early Intel Haswell CPU have AVX2 instructions, but do not have > > BMI1 and BMI2 instructions. Some AVX2 string functions only check for > > AVX2, but use BMI1, BMI2 or LZCNT instructions. This patchset tries to > > fix that. > > > > While most fixes only change ifunc-impl-list.c, and thus only concerns > > the testsuite, the strn(case)cmp is a real issue affecting early Intel > > str(case)cmp as well, correct? Oops, yes forgot to update the cover letter on that aspect. > > Haswell CPU, reported to affect Debian Sid and Fedora Rawhide. > > > > On the other hand, the check for LZCNT in memrchr is purely for > > correctness, I am not aware of a CPU implementing AVX2 without LZCNT. > > > > This has been tested by remplacing all BMI1 and BMI2 instructions in the > > source code by the "ud2" instruction and disabling the BMI1, BMI2 > > feature detection, and running the testsuite. > > > > Resolves: BZ #29611 > > > > Change v1 -> v2: > > - Better scan for BMI2 instructions (shlx and shrx) and BMI1 > > instructions (blsmsk) instructions following the feedback from Noah > > Goldstein > > > > Aurelien Jarno (6): > > x86: include BMI1 and BMI2 in x86-64-v3 level > > x86-64: Require BMI2 for AVX2 str*cmp and wcs(n)cmp implementations > > x86-64: Require BMI2 for AVX2 (raw|w)memchr implementations > > x86-64: Require LZCNT for AVX2 memrchr implementation > > x86-64: Require BMI1/BMI2 for AVX2 strrchr and wcsrchr implementations > > x86-64: Require BMI2 for AVX2 memrchr implementation > > > > sysdeps/x86/get-isa-level.h | 2 + > > sysdeps/x86/isa-level.h | 2 + > > sysdeps/x86_64/multiarch/ifunc-avx2.h | 2 + > > sysdeps/x86_64/multiarch/ifunc-impl-list.c | 86 ++++++++++++++++----- > > sysdeps/x86_64/multiarch/ifunc-strcasecmp.h | 1 + > > sysdeps/x86_64/multiarch/strcmp.c | 4 +- > > sysdeps/x86_64/multiarch/strncmp.c | 4 +- > > 7 files changed, 76 insertions(+), 25 deletions(-) > > > > -- > > 2.35.1 > > >
On Sun, Oct 2, 2022 at 2:09 PM Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2022-10-02 09:21, Noah Goldstein wrote: > > On Sun, Oct 2, 2022 at 5:34 AM Aurelien Jarno <aurelien@aurel32.net> wrote: > > > > > > Some early Intel Haswell CPU have AVX2 instructions, but do not have > > > BMI1 and BMI2 instructions. Some AVX2 string functions only check for > > > AVX2, but use BMI1, BMI2 or LZCNT instructions. This patchset tries to > > > fix that. > > > > > > While most fixes only change ifunc-impl-list.c, and thus only concerns > > > the testsuite, the strn(case)cmp is a real issue affecting early Intel > > > > str(case)cmp as well, correct? > > Oops, yes forgot to update the cover letter on that aspect. > > > > Haswell CPU, reported to affect Debian Sid and Fedora Rawhide. > > > > > > On the other hand, the check for LZCNT in memrchr is purely for > > > correctness, I am not aware of a CPU implementing AVX2 without LZCNT. > > > > > > This has been tested by remplacing all BMI1 and BMI2 instructions in the > > > source code by the "ud2" instruction and disabling the BMI1, BMI2 > > > feature detection, and running the testsuite. > > > > > > Resolves: BZ #29611 > > > > > > Change v1 -> v2: > > > - Better scan for BMI2 instructions (shlx and shrx) and BMI1 > > > instructions (blsmsk) instructions following the feedback from Noah > > > Goldstein > > > > > > Aurelien Jarno (6): > > > x86: include BMI1 and BMI2 in x86-64-v3 level > > > x86-64: Require BMI2 for AVX2 str*cmp and wcs(n)cmp implementations > > > x86-64: Require BMI2 for AVX2 (raw|w)memchr implementations > > > x86-64: Require LZCNT for AVX2 memrchr implementation > > > x86-64: Require BMI1/BMI2 for AVX2 strrchr and wcsrchr implementations > > > x86-64: Require BMI2 for AVX2 memrchr implementation > > > > > > sysdeps/x86/get-isa-level.h | 2 + > > > sysdeps/x86/isa-level.h | 2 + > > > sysdeps/x86_64/multiarch/ifunc-avx2.h | 2 + > > > sysdeps/x86_64/multiarch/ifunc-impl-list.c | 86 ++++++++++++++++----- > > > sysdeps/x86_64/multiarch/ifunc-strcasecmp.h | 1 + > > > sysdeps/x86_64/multiarch/strcmp.c | 4 +- > > > sysdeps/x86_64/multiarch/strncmp.c | 4 +- > > > 7 files changed, 76 insertions(+), 25 deletions(-) > > > > > > -- > > > 2.35.1 > > > > > > > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net Patchset looks good. Do you have commit permissions? If not I can push them for you. Thanks for the bugfix! NB: the str*(case)cmp, wcs(n)cmp bug affects 2.36, 2.35, 2.34, 2.33.
On 2022-10-02 17:11, Noah Goldstein via Libc-alpha wrote: > On Sun, Oct 2, 2022 at 2:09 PM Aurelien Jarno <aurelien@aurel32.net> wrote: > > > > On 2022-10-02 09:21, Noah Goldstein wrote: > > > On Sun, Oct 2, 2022 at 5:34 AM Aurelien Jarno <aurelien@aurel32.net> wrote: > > > > > > > > Some early Intel Haswell CPU have AVX2 instructions, but do not have > > > > BMI1 and BMI2 instructions. Some AVX2 string functions only check for > > > > AVX2, but use BMI1, BMI2 or LZCNT instructions. This patchset tries to > > > > fix that. > > > > > > > > While most fixes only change ifunc-impl-list.c, and thus only concerns > > > > the testsuite, the strn(case)cmp is a real issue affecting early Intel > > > > > > str(case)cmp as well, correct? > > > > Oops, yes forgot to update the cover letter on that aspect. > > > > > > Haswell CPU, reported to affect Debian Sid and Fedora Rawhide. > > > > > > > > On the other hand, the check for LZCNT in memrchr is purely for > > > > correctness, I am not aware of a CPU implementing AVX2 without LZCNT. > > > > > > > > This has been tested by remplacing all BMI1 and BMI2 instructions in the > > > > source code by the "ud2" instruction and disabling the BMI1, BMI2 > > > > feature detection, and running the testsuite. > > > > > > > > Resolves: BZ #29611 > > > > > > > > Change v1 -> v2: > > > > - Better scan for BMI2 instructions (shlx and shrx) and BMI1 > > > > instructions (blsmsk) instructions following the feedback from Noah > > > > Goldstein > > > > > > > > Aurelien Jarno (6): > > > > x86: include BMI1 and BMI2 in x86-64-v3 level > > > > x86-64: Require BMI2 for AVX2 str*cmp and wcs(n)cmp implementations > > > > x86-64: Require BMI2 for AVX2 (raw|w)memchr implementations > > > > x86-64: Require LZCNT for AVX2 memrchr implementation > > > > x86-64: Require BMI1/BMI2 for AVX2 strrchr and wcsrchr implementations > > > > x86-64: Require BMI2 for AVX2 memrchr implementation > > > > > > > > sysdeps/x86/get-isa-level.h | 2 + > > > > sysdeps/x86/isa-level.h | 2 + > > > > sysdeps/x86_64/multiarch/ifunc-avx2.h | 2 + > > > > sysdeps/x86_64/multiarch/ifunc-impl-list.c | 86 ++++++++++++++++----- > > > > sysdeps/x86_64/multiarch/ifunc-strcasecmp.h | 1 + > > > > sysdeps/x86_64/multiarch/strcmp.c | 4 +- > > > > sysdeps/x86_64/multiarch/strncmp.c | 4 +- > > > > 7 files changed, 76 insertions(+), 25 deletions(-) > > > > > > > > -- > > > > 2.35.1 > > > > > > > > > > > -- > > Aurelien Jarno GPG: 4096R/1DDD8C9B > > aurelien@aurel32.net http://www.aurel32.net > > Patchset looks good. > > Do you have commit permissions? If not I can push them for you. Yes, I can commit them, though I'll wait for v3 to be reviewed. > Thanks for the bugfix! > > NB: > the str*(case)cmp, wcs(n)cmp bug affects 2.36, 2.35, 2.34, 2.33. Yep, I have already prepared a backport of the whole patchset to those branches.
On Mon, Oct 3, 2022 at 10:36 AM Aurelien Jarno <aurelien@aurel32.net> wrote: > > On 2022-10-02 17:11, Noah Goldstein via Libc-alpha wrote: > > On Sun, Oct 2, 2022 at 2:09 PM Aurelien Jarno <aurelien@aurel32.net> wrote: > > > > > > On 2022-10-02 09:21, Noah Goldstein wrote: > > > > On Sun, Oct 2, 2022 at 5:34 AM Aurelien Jarno <aurelien@aurel32.net> wrote: > > > > > > > > > > Some early Intel Haswell CPU have AVX2 instructions, but do not have > > > > > BMI1 and BMI2 instructions. Some AVX2 string functions only check for > > > > > AVX2, but use BMI1, BMI2 or LZCNT instructions. This patchset tries to > > > > > fix that. > > > > > > > > > > While most fixes only change ifunc-impl-list.c, and thus only concerns > > > > > the testsuite, the strn(case)cmp is a real issue affecting early Intel > > > > > > > > str(case)cmp as well, correct? > > > > > > Oops, yes forgot to update the cover letter on that aspect. > > > > > > > > Haswell CPU, reported to affect Debian Sid and Fedora Rawhide. > > > > > > > > > > On the other hand, the check for LZCNT in memrchr is purely for > > > > > correctness, I am not aware of a CPU implementing AVX2 without LZCNT. > > > > > > > > > > This has been tested by remplacing all BMI1 and BMI2 instructions in the > > > > > source code by the "ud2" instruction and disabling the BMI1, BMI2 > > > > > feature detection, and running the testsuite. > > > > > > > > > > Resolves: BZ #29611 > > > > > > > > > > Change v1 -> v2: > > > > > - Better scan for BMI2 instructions (shlx and shrx) and BMI1 > > > > > instructions (blsmsk) instructions following the feedback from Noah > > > > > Goldstein > > > > > > > > > > Aurelien Jarno (6): > > > > > x86: include BMI1 and BMI2 in x86-64-v3 level > > > > > x86-64: Require BMI2 for AVX2 str*cmp and wcs(n)cmp implementations > > > > > x86-64: Require BMI2 for AVX2 (raw|w)memchr implementations > > > > > x86-64: Require LZCNT for AVX2 memrchr implementation > > > > > x86-64: Require BMI1/BMI2 for AVX2 strrchr and wcsrchr implementations > > > > > x86-64: Require BMI2 for AVX2 memrchr implementation > > > > > > > > > > sysdeps/x86/get-isa-level.h | 2 + > > > > > sysdeps/x86/isa-level.h | 2 + > > > > > sysdeps/x86_64/multiarch/ifunc-avx2.h | 2 + > > > > > sysdeps/x86_64/multiarch/ifunc-impl-list.c | 86 ++++++++++++++++----- > > > > > sysdeps/x86_64/multiarch/ifunc-strcasecmp.h | 1 + > > > > > sysdeps/x86_64/multiarch/strcmp.c | 4 +- > > > > > sysdeps/x86_64/multiarch/strncmp.c | 4 +- > > > > > 7 files changed, 76 insertions(+), 25 deletions(-) > > > > > > > > > > -- > > > > > 2.35.1 > > > > > > > > > > > > > > > -- > > > Aurelien Jarno GPG: 4096R/1DDD8C9B > > > aurelien@aurel32.net http://www.aurel32.net > > > > Patchset looks good. > > > > Do you have commit permissions? If not I can push them for you. > > Yes, I can commit them, though I'll wait for v3 to be reviewed. > > > Thanks for the bugfix! > > > > NB: > > the str*(case)cmp, wcs(n)cmp bug affects 2.36, 2.35, 2.34, 2.33. > > Yep, I have already prepared a backport of the whole patchset to those > branches. > If thats the case you may not need V3 because AFAIK thats the only reason to split the strcmp patches up. Sunil can you comment? > -- > Aurelien Jarno GPG: 4096R/1DDD8C9B > aurelien@aurel32.net http://www.aurel32.net