[PATCH] x86_64: Remove avx512 strstr implementation
Noah Goldstein
goldstein.w.n@gmail.com
Thu Mar 21 23:17:17 GMT 2024
On Thu, Mar 21, 2024 at 12:12 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
> As indicated in a recent thread, this it is a simple brute-force
> algorithm that checks the whole needle at a matching character pair
> (and does so 1 byte at a time after the first 64 bytes of a needle).
> Also it never skips ahead and thus can match at every haystack
> position after trying to match all of the needle, which generic
> implementation avoids.
>
> As indicated by Wilco, a 4x larger needle and 16x larger haystack gives
> a clear 65x slowdown both basic_strstr and __strstr_avx512:
>
> "ifuncs": ["basic_strstr", "twoway_strstr", "__strstr_avx512",
> "__strstr_sse2_unaligned", "__strstr_generic"],
>
> {
> "len_haystack": 65536,
> "len_needle": 1024,
> "align_haystack": 0,
> "align_needle": 0,
> "fail": 1,
> "desc": "Difficult bruteforce needle",
> "timings": [4.0948e+07, 15094.5, 3.20818e+07, 108558, 10839.2]
> },
> {
> "len_haystack": 1048576,
> "len_needle": 4096,
> "align_haystack": 0,
> "align_needle": 0,
> "fail": 1,
> "desc": "Difficult bruteforce needle",
> "timings": [2.69767e+09, 100797, 2.08535e+09, 495706, 82666.9]
> }
>
> PS: I don't have an AVX512 capable machine to verify this issues, but
> skimming through the code it does seems to follow what Wilco has
> described.
>
> ---
> sysdeps/x86_64/multiarch/Makefile | 3 -
> sysdeps/x86_64/multiarch/ifunc-impl-list.c | 6 -
> sysdeps/x86_64/multiarch/strstr-avx512.c | 218 ---------------------
> sysdeps/x86_64/multiarch/strstr.c | 25 +--
> 4 files changed, 4 insertions(+), 248 deletions(-)
> delete mode 100644 sysdeps/x86_64/multiarch/strstr-avx512.c
>
> diff --git a/sysdeps/x86_64/multiarch/Makefile b/sysdeps/x86_64/multiarch/Makefile
> index d3d2270394..696cb66991 100644
> --- a/sysdeps/x86_64/multiarch/Makefile
> +++ b/sysdeps/x86_64/multiarch/Makefile
> @@ -117,7 +117,6 @@ sysdep_routines += \
> strrchr-evex512 \
> strrchr-sse2 \
> strspn-sse4 \
> - strstr-avx512 \
> strstr-sse2-unaligned \
> varshift \
> # sysdep_routines
> @@ -125,8 +124,6 @@ sysdep_routines += \
> CFLAGS-strcspn-sse4.c += -msse4
> CFLAGS-strpbrk-sse4.c += -msse4
> CFLAGS-strspn-sse4.c += -msse4
> -
> -CFLAGS-strstr-avx512.c += -mavx512f -mavx512vl -mavx512dq -mavx512bw -mbmi -mbmi2 -O3
> endif
>
> ifeq ($(subdir),wcsmbs)
> diff --git a/sysdeps/x86_64/multiarch/ifunc-impl-list.c b/sysdeps/x86_64/multiarch/ifunc-impl-list.c
> index c4a21d4b7c..0bbb71bbbf 100644
> --- a/sysdeps/x86_64/multiarch/ifunc-impl-list.c
> +++ b/sysdeps/x86_64/multiarch/ifunc-impl-list.c
> @@ -790,12 +790,6 @@ __libc_ifunc_impl_list (const char *name, struct libc_ifunc_impl *array,
>
> /* Support sysdeps/x86_64/multiarch/strstr.c. */
> IFUNC_IMPL (i, name, strstr,
> - IFUNC_IMPL_ADD (array, i, strstr,
> - (CPU_FEATURE_USABLE (AVX512VL)
> - && CPU_FEATURE_USABLE (AVX512BW)
> - && CPU_FEATURE_USABLE (AVX512DQ)
> - && CPU_FEATURE_USABLE (BMI2)),
> - __strstr_avx512)
> IFUNC_IMPL_ADD (array, i, strstr, 1, __strstr_sse2_unaligned)
> IFUNC_IMPL_ADD (array, i, strstr, 1, __strstr_generic))
>
> diff --git a/sysdeps/x86_64/multiarch/strstr-avx512.c b/sysdeps/x86_64/multiarch/strstr-avx512.c
> deleted file mode 100644
> index 3ac53accbd..0000000000
> --- a/sysdeps/x86_64/multiarch/strstr-avx512.c
> +++ /dev/null
> @@ -1,218 +0,0 @@
> -/* strstr optimized with 512-bit AVX-512 instructions
> - Copyright (C) 2022-2024 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> -
> - The GNU C Library is free software; you can redistribute it and/or
> - modify it under the terms of the GNU Lesser General Public
> - License as published by the Free Software Foundation; either
> - version 2.1 of the License, or (at your option) any later version.
> -
> - The GNU C Library is distributed in the hope that it will be useful,
> - but WITHOUT ANY WARRANTY; without even the implied warranty of
> - MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> - Lesser General Public License for more details.
> -
> - You should have received a copy of the GNU Lesser General Public
> - License along with the GNU C Library; if not, see
> - <https://www.gnu.org/licenses/>. */
> -
> -#include <immintrin.h>
> -#include <inttypes.h>
> -#include <stdbool.h>
> -#include <string.h>
> -
> -#define FULL_MMASK64 0xffffffffffffffff
> -#define ONE_64BIT 0x1ull
> -#define ZMM_SIZE_IN_BYTES 64
> -#define PAGESIZE 4096
> -
> -#define cvtmask64_u64(...) (uint64_t) (__VA_ARGS__)
> -#define kshiftri_mask64(x, y) ((x) >> (y))
> -#define kand_mask64(x, y) ((x) & (y))
> -
> -/*
> - Returns the index of the first edge within the needle, returns 0 if no edge
> - is found. Example: 'ab' is the first edge in 'aaaaaaaaaabaarddg'
> - */
> -static inline size_t
> -find_edge_in_needle (const char *ned)
> -{
> - size_t ind = 0;
> - while (ned[ind + 1] != '\0')
> - {
> - if (ned[ind] != ned[ind + 1])
> - return ind;
> - else
> - ind = ind + 1;
> - }
> - return 0;
> -}
> -
> -/*
> - Compare needle with haystack byte by byte at specified location
> - */
> -static inline bool
> -verify_string_match (const char *hay, const size_t hay_index, const char *ned,
> - size_t ind)
> -{
> - while (ned[ind] != '\0')
> - {
> - if (ned[ind] != hay[hay_index + ind])
> - return false;
> - ind = ind + 1;
> - }
> - return true;
> -}
> -
> -/*
> - Compare needle with haystack at specified location. The first 64 bytes are
> - compared using a ZMM register.
> - */
> -static inline bool
> -verify_string_match_avx512 (const char *hay, const size_t hay_index,
> - const char *ned, const __mmask64 ned_mask,
> - const __m512i ned_zmm)
> -{
> - /* check first 64 bytes using zmm and then scalar */
> - __m512i hay_zmm = _mm512_loadu_si512 (hay + hay_index); // safe to do so
> - __mmask64 match = _mm512_mask_cmpneq_epi8_mask (ned_mask, hay_zmm, ned_zmm);
> - if (match != 0x0) // failed the first few chars
> - return false;
> - else if (ned_mask == FULL_MMASK64)
> - return verify_string_match (hay, hay_index, ned, ZMM_SIZE_IN_BYTES);
> - return true;
> -}
> -
> -char *
> -__strstr_avx512 (const char *haystack, const char *ned)
> -{
> - char first = ned[0];
> - if (first == '\0')
> - return (char *)haystack;
> - if (ned[1] == '\0')
> - return (char *)strchr (haystack, ned[0]);
> -
> - size_t edge = find_edge_in_needle (ned);
> -
> - /* ensure haystack is as long as the pos of edge in needle */
> - for (int ii = 0; ii < edge; ++ii)
> - {
> - if (haystack[ii] == '\0')
> - return NULL;
> - }
> -
> - /*
> - Load 64 bytes of the needle and save it to a zmm register
> - Read one cache line at a time to avoid loading across a page boundary
> - */
> - __mmask64 ned_load_mask = _bzhi_u64 (
> - FULL_MMASK64, 64 - ((uintptr_t) (ned) & 63));
> - __m512i ned_zmm = _mm512_maskz_loadu_epi8 (ned_load_mask, ned);
> - __mmask64 ned_nullmask
> - = _mm512_mask_testn_epi8_mask (ned_load_mask, ned_zmm, ned_zmm);
> -
> - if (__glibc_unlikely (ned_nullmask == 0x0))
> - {
> - ned_zmm = _mm512_loadu_si512 (ned);
> - ned_nullmask = _mm512_testn_epi8_mask (ned_zmm, ned_zmm);
> - ned_load_mask = ned_nullmask ^ (ned_nullmask - ONE_64BIT);
> - if (ned_nullmask != 0x0)
> - ned_load_mask = ned_load_mask >> 1;
> - }
> - else
> - {
> - ned_load_mask = ned_nullmask ^ (ned_nullmask - ONE_64BIT);
> - ned_load_mask = ned_load_mask >> 1;
> - }
> - const __m512i ned0 = _mm512_set1_epi8 (ned[edge]);
> - const __m512i ned1 = _mm512_set1_epi8 (ned[edge + 1]);
> -
> - /*
> - Read the bytes of haystack in the current cache line
> - */
> - size_t hay_index = edge;
> - __mmask64 loadmask = _bzhi_u64 (
> - FULL_MMASK64, 64 - ((uintptr_t) (haystack + hay_index) & 63));
> - /* First load is a partial cache line */
> - __m512i hay0 = _mm512_maskz_loadu_epi8 (loadmask, haystack + hay_index);
> - /* Search for NULL and compare only till null char */
> - uint64_t nullmask
> - = cvtmask64_u64 (_mm512_mask_testn_epi8_mask (loadmask, hay0, hay0));
> - uint64_t cmpmask = nullmask ^ (nullmask - ONE_64BIT);
> - cmpmask = cmpmask & cvtmask64_u64 (loadmask);
> - /* Search for the 2 characters of needle */
> - __mmask64 k0 = _mm512_cmpeq_epi8_mask (hay0, ned0);
> - __mmask64 k1 = _mm512_cmpeq_epi8_mask (hay0, ned1);
> - k1 = kshiftri_mask64 (k1, 1);
> - /* k2 masks tell us if both chars from needle match */
> - uint64_t k2 = cvtmask64_u64 (kand_mask64 (k0, k1)) & cmpmask;
> - /* For every match, search for the entire needle for a full match */
> - while (k2)
> - {
> - uint64_t bitcount = _tzcnt_u64 (k2);
> - k2 = _blsr_u64 (k2);
> - size_t match_pos = hay_index + bitcount - edge;
> - if (((uintptr_t) (haystack + match_pos) & (PAGESIZE - 1))
> - < PAGESIZE - 1 - ZMM_SIZE_IN_BYTES)
> - {
> - /*
> - * Use vector compare as long as you are not crossing a page
> - */
> - if (verify_string_match_avx512 (haystack, match_pos, ned,
> - ned_load_mask, ned_zmm))
> - return (char *)haystack + match_pos;
> - }
> - else
> - {
> - if (verify_string_match (haystack, match_pos, ned, 0))
> - return (char *)haystack + match_pos;
> - }
> - }
> - /* We haven't checked for potential match at the last char yet */
> - haystack = (const char *)(((uintptr_t) (haystack + hay_index) | 63));
> - hay_index = 0;
> -
> - /*
> - Loop over one cache line at a time to prevent reading over page
> - boundary
> - */
> - __m512i hay1;
> - while (nullmask == 0)
> - {
> - hay0 = _mm512_loadu_si512 (haystack + hay_index);
> - hay1 = _mm512_load_si512 (haystack + hay_index
> - + 1); // Always 64 byte aligned
> - nullmask = cvtmask64_u64 (_mm512_testn_epi8_mask (hay1, hay1));
> - /* Compare only till null char */
> - cmpmask = nullmask ^ (nullmask - ONE_64BIT);
> - k0 = _mm512_cmpeq_epi8_mask (hay0, ned0);
> - k1 = _mm512_cmpeq_epi8_mask (hay1, ned1);
> - /* k2 masks tell us if both chars from needle match */
> - k2 = cvtmask64_u64 (kand_mask64 (k0, k1)) & cmpmask;
> - /* For every match, compare full strings for potential match */
> - while (k2)
> - {
> - uint64_t bitcount = _tzcnt_u64 (k2);
> - k2 = _blsr_u64 (k2);
> - size_t match_pos = hay_index + bitcount - edge;
> - if (((uintptr_t) (haystack + match_pos) & (PAGESIZE - 1))
> - < PAGESIZE - 1 - ZMM_SIZE_IN_BYTES)
> - {
> - /*
> - * Use vector compare as long as you are not crossing a page
> - */
> - if (verify_string_match_avx512 (haystack, match_pos, ned,
> - ned_load_mask, ned_zmm))
> - return (char *)haystack + match_pos;
> - }
> - else
> - {
> - /* Compare byte by byte */
> - if (verify_string_match (haystack, match_pos, ned, 0))
> - return (char *)haystack + match_pos;
> - }
> - }
> - hay_index += ZMM_SIZE_IN_BYTES;
> - }
> - return NULL;
> -}
> diff --git a/sysdeps/x86_64/multiarch/strstr.c b/sysdeps/x86_64/multiarch/strstr.c
> index a513bac5c3..828308668b 100644
> --- a/sysdeps/x86_64/multiarch/strstr.c
> +++ b/sysdeps/x86_64/multiarch/strstr.c
> @@ -35,32 +35,15 @@
>
> extern __typeof (__redirect_strstr) __strstr_sse2_unaligned attribute_hidden;
> extern __typeof (__redirect_strstr) __strstr_generic attribute_hidden;
> -extern __typeof (__redirect_strstr) __strstr_avx512 attribute_hidden;
>
> #include "init-arch.h"
>
> /* Avoid DWARF definition DIE on ifunc symbol so that GDB can handle
> ifunc symbol properly. */
> extern __typeof (__redirect_strstr) __libc_strstr;
> -
> -static inline void *
> -IFUNC_SELECTOR (void)
> -{
> - const struct cpu_features *cpu_features = __get_cpu_features ();
> -
> - if (!CPU_FEATURES_ARCH_P (cpu_features, Prefer_No_AVX512)
> - && CPU_FEATURE_USABLE_P (cpu_features, AVX512VL)
> - && CPU_FEATURE_USABLE_P (cpu_features, AVX512BW)
> - && CPU_FEATURE_USABLE_P (cpu_features, AVX512DQ)
> - && CPU_FEATURE_USABLE_P (cpu_features, BMI2))
> - return __strstr_avx512;
> -
> - if (CPU_FEATURES_ARCH_P (cpu_features, Fast_Unaligned_Load))
> - return __strstr_sse2_unaligned;
> -
> - return __strstr_generic;
> -}
> -
> -libc_ifunc_redirected (__redirect_strstr, __libc_strstr, IFUNC_SELECTOR ());
> +libc_ifunc (__libc_strstr,
> + HAS_ARCH_FEATURE (Fast_Unaligned_Load)
> + ? __strstr_sse2_unaligned
> + : __strstr_generic)
> #undef strstr
> strong_alias (__libc_strstr, strstr)
> --
> 2.34.1
>
LGTM.
Reviewed-by: Noah Goldstein <goldstein.w.n@gmail.com>
More information about the Libc-alpha
mailing list