diff mbox series

[v3] x86: Fix unchecked AVX512-VBMI2 usage in strrchr-evex-base.S

Message ID 20231109224918.2036632-1-goldstein.w.n@gmail.com
State New
Headers show
Series [v3] x86: Fix unchecked AVX512-VBMI2 usage in strrchr-evex-base.S | expand

Commit Message

Noah Goldstein Nov. 9, 2023, 10:49 p.m. UTC
strrchr-evex-base used `vpcompress{b|d}` in the page cross logic but
was missing the CPU_FEATURE checks for VBMI2 in the
ifunc/ifunc-impl-list.

The fix is either to add those checks or change the logic to not use
`vpcompress{b|d}`. Choosing the latter here so that the strrchr-evex
implementation is usable on SKX.

New implementation is a bit slower, but this is in a cold path so its
probably okay.
---
 sysdeps/x86_64/multiarch/strrchr-evex-base.S | 61 ++++++++++++--------
 1 file changed, 37 insertions(+), 24 deletions(-)

Comments

Sunil Pandey Nov. 10, 2023, 3:10 a.m. UTC | #1
On Thu, Nov 9, 2023 at 2:49 PM Noah Goldstein <goldstein.w.n@gmail.com>
wrote:

> strrchr-evex-base used `vpcompress{b|d}` in the page cross logic but
> was missing the CPU_FEATURE checks for VBMI2 in the
> ifunc/ifunc-impl-list.
>
> The fix is either to add those checks or change the logic to not use
> `vpcompress{b|d}`. Choosing the latter here so that the strrchr-evex
> implementation is usable on SKX.
>
> New implementation is a bit slower, but this is in a cold path so its
> probably okay.
> ---
>  sysdeps/x86_64/multiarch/strrchr-evex-base.S | 61 ++++++++++++--------
>  1 file changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> index cd6a0a870a..eb5f1f855a 100644
> --- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> +++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> @@ -35,7 +35,6 @@
>  #  define CHAR_SIZE    4
>  #  define VPCMP                vpcmpd
>  #  define VPMIN                vpminud
> -#  define VPCOMPRESS   vpcompressd
>  #  define VPTESTN      vptestnmd
>  #  define VPTEST       vptestmd
>  #  define VPBROADCAST  vpbroadcastd
> @@ -46,7 +45,6 @@
>  #  define CHAR_SIZE    1
>  #  define VPCMP                vpcmpb
>  #  define VPMIN                vpminub
> -#  define VPCOMPRESS   vpcompressb
>  #  define VPTESTN      vptestnmb
>  #  define VPTEST       vptestmb
>  #  define VPBROADCAST  vpbroadcastb
> @@ -71,7 +69,7 @@ ENTRY_P2ALIGN(STRRCHR, 6)
>         andl    $(PAGE_SIZE - 1), %eax
>         cmpl    $(PAGE_SIZE - VEC_SIZE), %eax
>         jg      L(cross_page_boundary)
> -
> +L(page_cross_continue):
>         VMOVU   (%rdi), %VMM(1)
>         /* k0 has a 1 for each zero CHAR in YMM1.  */
>         VPTESTN %VMM(1), %VMM(1), %k0
> @@ -79,10 +77,11 @@ ENTRY_P2ALIGN(STRRCHR, 6)
>         test    %VGPR(rsi), %VGPR(rsi)
>         jz      L(aligned_more)
>         /* fallthrough: zero CHAR in first VEC.  */
> -L(page_cross_return):
> +
>         /* K1 has a 1 for each search CHAR match in VEC(1).  */
>         VPCMPEQ %VMATCH, %VMM(1), %k1
>         KMOV    %k1, %VGPR(rax)
> +L(page_cross_return):
>

Unused label.


>         /* Build mask up until first zero CHAR (used to mask of
>            potential search CHAR matches past the end of the string).  */
>         blsmsk  %VGPR(rsi), %VGPR(rsi)
> @@ -167,7 +166,6 @@ L(first_vec_x1_return):
>
>         .p2align 4,, 12
>  L(aligned_more):
> -L(page_cross_continue):
>         /* Need to keep original pointer incase VEC(1) has last match.  */
>         movq    %rdi, %rsi
>         andq    $-VEC_SIZE, %rdi
> @@ -340,34 +338,49 @@ L(return_new_match_ret):
>         leaq    (VEC_SIZE * 2)(%rdi, %rax, CHAR_SIZE), %rax
>         ret
>
> -       .p2align 4,, 4
>  L(cross_page_boundary):
> +       /* eax contains all the page offset bits of src (rdi). `xor rdi,
> +          rax` sets pointer will all page offset bits cleared so
> +          offset of (PAGE_SIZE - VEC_SIZE) will get last aligned VEC
> +          before page cross (guaranteed to be safe to read). Doing this
> +          as opposed to `movq %rdi, %rax; andq $-VEC_SIZE, %rax` saves
> +          a bit of code size.  */
>         xorq    %rdi, %rax
> -       mov     $-1, %VRDX
> -       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(6)
> -       VPTESTN %VMM(6), %VMM(6), %k0
> +       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(1)
> +       VPTESTN %VMM(1), %VMM(1), %k0
>         KMOV    %k0, %VRSI
>
> +       /* Shift out zero CHAR matches that are before the beginning of
> +          src (rdi).  */
>  # ifdef USE_AS_WCSRCHR
>         movl    %edi, %ecx
> -       and     $(VEC_SIZE - 1), %ecx
> +       andl    $(VEC_SIZE - 1), %ecx
>         shrl    $2, %ecx
>  # endif
> -       shlx    %SHIFT_REG, %VRDX, %VRDX
> -
> -# ifdef USE_AS_WCSRCHR
> -       kmovw   %edx, %k1
> -# else
> -       KMOV    %VRDX, %k1
> -# endif
> -
> -       VPCOMPRESS %VMM(6), %VMM(1){%k1}{z}
> -       /* We could technically just jmp back after the vpcompress but
> -          it doesn't save any 16-byte blocks.  */
>         shrx    %SHIFT_REG, %VRSI, %VRSI
> +
>         test    %VRSI, %VRSI
> -       jnz     L(page_cross_return)
> -       jmp     L(page_cross_continue)
> -       /* 1-byte from cache line.  */
> +       jz      L(page_cross_continue)
> +
> +       /* Found zero CHAR so need to test for search CHAR.  */
> +       VPCMP   $0, %VMATCH, %VMM(1), %k1
> +       KMOV    %k1, %VRAX
> +       /* Shift out search CHAR matches that are before the beginning of
> +          src (rdi).  */
> +       shrx    %SHIFT_REG, %VRAX, %VRAX
> +       /* Check if any search CHAR match in range.  */
> +       blsmsk  %VRSI, %VRSI
> +       and     %VRSI, %VRAX
> +       jz      L(ret2)
> +       bsr     %VRAX, %VRAX
> +#  ifdef USE_AS_WCSRCHR
> +       leaq    (%rdi, %rax, CHAR_SIZE), %rax
> +#  else
> +       addq    %rdi, %rax
> +#  endif
> +L(ret2):
> +       ret
>

Modified instructions to fit page crossing logic in cache line for
strrchr-evex512.

 300: 48 31 f8             xor    %rdi,%rax
 303: 62 e1 fe 48 6f 48 3f vmovdqu64 0xfc0(%rax),%zmm17
 30a: 62 b2 76 40 26 c1     vptestnmb %zmm17,%zmm17,%k0
 310: c4 e1 fb 93 f0       kmovq  %k0,%rsi
 315: 89 f9                 mov    %edi,%ecx
 317: 48 d3 ee             shr    %cl,%rsi
 31a: 0f 84 f8 fc ff ff     je     18 <__strrchr_evex512+0x18>
 320: 62 b1 75 40 74 c8     vpcmpeqb %zmm16,%zmm17,%k1
 326: c4 e1 fb 93 c1       kmovq  %k1,%rax
 32b: 48 d3 e8             shr    %cl,%rax
 32e: c4 e2 c8 f3 d6       blsmsk %rsi,%rsi
 333: 48 21 f0             and    %rsi,%rax
 336: 74 07                 je     33f <__strrchr_evex512+0x33f>
 338: 48 0f bd c0           bsr    %rax,%rax
 33c: 48 01 f8             add    %rdi,%rax
 33f: c3                   ret


> +       /* 3 bytes from cache-line for evex.
> +          Crosses cache line for evex512.  */
>  END(STRRCHR)
>  #endif
> --
> 2.34.1
>
>
Noah Goldstein Nov. 11, 2023, 3:21 a.m. UTC | #2
On Thu, Nov 9, 2023 at 9:11 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>
>
>
> On Thu, Nov 9, 2023 at 2:49 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>
>> strrchr-evex-base used `vpcompress{b|d}` in the page cross logic but
>> was missing the CPU_FEATURE checks for VBMI2 in the
>> ifunc/ifunc-impl-list.
>>
>> The fix is either to add those checks or change the logic to not use
>> `vpcompress{b|d}`. Choosing the latter here so that the strrchr-evex
>> implementation is usable on SKX.
>>
>> New implementation is a bit slower, but this is in a cold path so its
>> probably okay.
>> ---
>>  sysdeps/x86_64/multiarch/strrchr-evex-base.S | 61 ++++++++++++--------
>>  1 file changed, 37 insertions(+), 24 deletions(-)
>>
>> diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> index cd6a0a870a..eb5f1f855a 100644
>> --- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> +++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> @@ -35,7 +35,6 @@
>>  #  define CHAR_SIZE    4
>>  #  define VPCMP                vpcmpd
>>  #  define VPMIN                vpminud
>> -#  define VPCOMPRESS   vpcompressd
>>  #  define VPTESTN      vptestnmd
>>  #  define VPTEST       vptestmd
>>  #  define VPBROADCAST  vpbroadcastd
>> @@ -46,7 +45,6 @@
>>  #  define CHAR_SIZE    1
>>  #  define VPCMP                vpcmpb
>>  #  define VPMIN                vpminub
>> -#  define VPCOMPRESS   vpcompressb
>>  #  define VPTESTN      vptestnmb
>>  #  define VPTEST       vptestmb
>>  #  define VPBROADCAST  vpbroadcastb
>> @@ -71,7 +69,7 @@ ENTRY_P2ALIGN(STRRCHR, 6)
>>         andl    $(PAGE_SIZE - 1), %eax
>>         cmpl    $(PAGE_SIZE - VEC_SIZE), %eax
>>         jg      L(cross_page_boundary)
>> -
>> +L(page_cross_continue):
>>         VMOVU   (%rdi), %VMM(1)
>>         /* k0 has a 1 for each zero CHAR in YMM1.  */
>>         VPTESTN %VMM(1), %VMM(1), %k0
>> @@ -79,10 +77,11 @@ ENTRY_P2ALIGN(STRRCHR, 6)
>>         test    %VGPR(rsi), %VGPR(rsi)
>>         jz      L(aligned_more)
>>         /* fallthrough: zero CHAR in first VEC.  */
>> -L(page_cross_return):
>> +
>>         /* K1 has a 1 for each search CHAR match in VEC(1).  */
>>         VPCMPEQ %VMATCH, %VMM(1), %k1
>>         KMOV    %k1, %VGPR(rax)
>> +L(page_cross_return):
>
>
> Unused label.
>
>>
>>         /* Build mask up until first zero CHAR (used to mask of
>>            potential search CHAR matches past the end of the string).  */
>>         blsmsk  %VGPR(rsi), %VGPR(rsi)
>> @@ -167,7 +166,6 @@ L(first_vec_x1_return):
>>
>>         .p2align 4,, 12
>>  L(aligned_more):
>> -L(page_cross_continue):
>>         /* Need to keep original pointer incase VEC(1) has last match.  */
>>         movq    %rdi, %rsi
>>         andq    $-VEC_SIZE, %rdi
>> @@ -340,34 +338,49 @@ L(return_new_match_ret):
>>         leaq    (VEC_SIZE * 2)(%rdi, %rax, CHAR_SIZE), %rax
>>         ret
>>
>> -       .p2align 4,, 4
>>  L(cross_page_boundary):
>> +       /* eax contains all the page offset bits of src (rdi). `xor rdi,
>> +          rax` sets pointer will all page offset bits cleared so
>> +          offset of (PAGE_SIZE - VEC_SIZE) will get last aligned VEC
>> +          before page cross (guaranteed to be safe to read). Doing this
>> +          as opposed to `movq %rdi, %rax; andq $-VEC_SIZE, %rax` saves
>> +          a bit of code size.  */
>>         xorq    %rdi, %rax
>> -       mov     $-1, %VRDX
>> -       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(6)
>> -       VPTESTN %VMM(6), %VMM(6), %k0
>> +       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(1)
>> +       VPTESTN %VMM(1), %VMM(1), %k0
>>         KMOV    %k0, %VRSI
>>
>> +       /* Shift out zero CHAR matches that are before the beginning of
>> +          src (rdi).  */
>>  # ifdef USE_AS_WCSRCHR
>>         movl    %edi, %ecx
>> -       and     $(VEC_SIZE - 1), %ecx
>> +       andl    $(VEC_SIZE - 1), %ecx
>>         shrl    $2, %ecx
>>  # endif
>> -       shlx    %SHIFT_REG, %VRDX, %VRDX
>> -
>> -# ifdef USE_AS_WCSRCHR
>> -       kmovw   %edx, %k1
>> -# else
>> -       KMOV    %VRDX, %k1
>> -# endif
>> -
>> -       VPCOMPRESS %VMM(6), %VMM(1){%k1}{z}
>> -       /* We could technically just jmp back after the vpcompress but
>> -          it doesn't save any 16-byte blocks.  */
>>         shrx    %SHIFT_REG, %VRSI, %VRSI
>> +
>>         test    %VRSI, %VRSI
>> -       jnz     L(page_cross_return)
>> -       jmp     L(page_cross_continue)
>> -       /* 1-byte from cache line.  */
>> +       jz      L(page_cross_continue)
>> +
>> +       /* Found zero CHAR so need to test for search CHAR.  */
>> +       VPCMP   $0, %VMATCH, %VMM(1), %k1
>> +       KMOV    %k1, %VRAX
>> +       /* Shift out search CHAR matches that are before the beginning of
>> +          src (rdi).  */
>> +       shrx    %SHIFT_REG, %VRAX, %VRAX
>> +       /* Check if any search CHAR match in range.  */
>> +       blsmsk  %VRSI, %VRSI
>> +       and     %VRSI, %VRAX
>> +       jz      L(ret2)
>> +       bsr     %VRAX, %VRAX
>> +#  ifdef USE_AS_WCSRCHR
>> +       leaq    (%rdi, %rax, CHAR_SIZE), %rax
>> +#  else
>> +       addq    %rdi, %rax
>> +#  endif
>> +L(ret2):
>> +       ret
>
>
> Modified instructions to fit page crossing logic in cache line for strrchr-evex512.
>
>  300: 48 31 f8             xor    %rdi,%rax
>  303: 62 e1 fe 48 6f 48 3f vmovdqu64 0xfc0(%rax),%zmm17
>  30a: 62 b2 76 40 26 c1     vptestnmb %zmm17,%zmm17,%k0
>  310: c4 e1 fb 93 f0       kmovq  %k0,%rsi
>  315: 89 f9                 mov    %edi,%ecx
>  317: 48 d3 ee             shr    %cl,%rsi
>  31a: 0f 84 f8 fc ff ff     je     18 <__strrchr_evex512+0x18>
>  320: 62 b1 75 40 74 c8     vpcmpeqb %zmm16,%zmm17,%k1
>  326: c4 e1 fb 93 c1       kmovq  %k1,%rax
>  32b: 48 d3 e8             shr    %cl,%rax
>  32e: c4 e2 c8 f3 d6       blsmsk %rsi,%rsi
>  333: 48 21 f0             and    %rsi,%rax
>  336: 74 07                 je     33f <__strrchr_evex512+0x33f>
>  338: 48 0f bd c0           bsr    %rax,%rax
>  33c: 48 01 f8             add    %rdi,%rax
>  33f: c3                   ret
>
Can update if you want, but its a alot of ifdefs, LMK.
>>
>> +       /* 3 bytes from cache-line for evex.
>> +          Crosses cache line for evex512.  */
>>  END(STRRCHR)
>>  #endif
>> --
>> 2.34.1
>>
Sunil Pandey Nov. 12, 2023, 5:56 p.m. UTC | #3
On Fri, Nov 10, 2023 at 7:21 PM Noah Goldstein <goldstein.w.n@gmail.com>
wrote:

> On Thu, Nov 9, 2023 at 9:11 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
> >
> >
> >
> > On Thu, Nov 9, 2023 at 2:49 PM Noah Goldstein <goldstein.w.n@gmail.com>
> wrote:
> >>
> >> strrchr-evex-base used `vpcompress{b|d}` in the page cross logic but
> >> was missing the CPU_FEATURE checks for VBMI2 in the
> >> ifunc/ifunc-impl-list.
> >>
> >> The fix is either to add those checks or change the logic to not use
> >> `vpcompress{b|d}`. Choosing the latter here so that the strrchr-evex
> >> implementation is usable on SKX.
> >>
> >> New implementation is a bit slower, but this is in a cold path so its
> >> probably okay.
> >> ---
> >>  sysdeps/x86_64/multiarch/strrchr-evex-base.S | 61 ++++++++++++--------
> >>  1 file changed, 37 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> >> index cd6a0a870a..eb5f1f855a 100644
> >> --- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> >> +++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> >> @@ -35,7 +35,6 @@
> >>  #  define CHAR_SIZE    4
> >>  #  define VPCMP                vpcmpd
> >>  #  define VPMIN                vpminud
> >> -#  define VPCOMPRESS   vpcompressd
> >>  #  define VPTESTN      vptestnmd
> >>  #  define VPTEST       vptestmd
> >>  #  define VPBROADCAST  vpbroadcastd
> >> @@ -46,7 +45,6 @@
> >>  #  define CHAR_SIZE    1
> >>  #  define VPCMP                vpcmpb
> >>  #  define VPMIN                vpminub
> >> -#  define VPCOMPRESS   vpcompressb
> >>  #  define VPTESTN      vptestnmb
> >>  #  define VPTEST       vptestmb
> >>  #  define VPBROADCAST  vpbroadcastb
> >> @@ -71,7 +69,7 @@ ENTRY_P2ALIGN(STRRCHR, 6)
> >>         andl    $(PAGE_SIZE - 1), %eax
> >>         cmpl    $(PAGE_SIZE - VEC_SIZE), %eax
> >>         jg      L(cross_page_boundary)
> >> -
> >> +L(page_cross_continue):
> >>         VMOVU   (%rdi), %VMM(1)
> >>         /* k0 has a 1 for each zero CHAR in YMM1.  */
> >>         VPTESTN %VMM(1), %VMM(1), %k0
> >> @@ -79,10 +77,11 @@ ENTRY_P2ALIGN(STRRCHR, 6)
> >>         test    %VGPR(rsi), %VGPR(rsi)
> >>         jz      L(aligned_more)
> >>         /* fallthrough: zero CHAR in first VEC.  */
> >> -L(page_cross_return):
> >> +
> >>         /* K1 has a 1 for each search CHAR match in VEC(1).  */
> >>         VPCMPEQ %VMATCH, %VMM(1), %k1
> >>         KMOV    %k1, %VGPR(rax)
> >> +L(page_cross_return):
> >
> >
> > Unused label.
> >
> >>
> >>         /* Build mask up until first zero CHAR (used to mask of
> >>            potential search CHAR matches past the end of the string).
> */
> >>         blsmsk  %VGPR(rsi), %VGPR(rsi)
> >> @@ -167,7 +166,6 @@ L(first_vec_x1_return):
> >>
> >>         .p2align 4,, 12
> >>  L(aligned_more):
> >> -L(page_cross_continue):
> >>         /* Need to keep original pointer incase VEC(1) has last match.
> */
> >>         movq    %rdi, %rsi
> >>         andq    $-VEC_SIZE, %rdi
> >> @@ -340,34 +338,49 @@ L(return_new_match_ret):
> >>         leaq    (VEC_SIZE * 2)(%rdi, %rax, CHAR_SIZE), %rax
> >>         ret
> >>
> >> -       .p2align 4,, 4
> >>  L(cross_page_boundary):
> >> +       /* eax contains all the page offset bits of src (rdi). `xor rdi,
> >> +          rax` sets pointer will all page offset bits cleared so
> >> +          offset of (PAGE_SIZE - VEC_SIZE) will get last aligned VEC
> >> +          before page cross (guaranteed to be safe to read). Doing this
> >> +          as opposed to `movq %rdi, %rax; andq $-VEC_SIZE, %rax` saves
> >> +          a bit of code size.  */
> >>         xorq    %rdi, %rax
> >> -       mov     $-1, %VRDX
> >> -       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(6)
> >> -       VPTESTN %VMM(6), %VMM(6), %k0
> >> +       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(1)
> >> +       VPTESTN %VMM(1), %VMM(1), %k0
> >>         KMOV    %k0, %VRSI
> >>
> >> +       /* Shift out zero CHAR matches that are before the beginning of
> >> +          src (rdi).  */
> >>  # ifdef USE_AS_WCSRCHR
> >>         movl    %edi, %ecx
> >> -       and     $(VEC_SIZE - 1), %ecx
> >> +       andl    $(VEC_SIZE - 1), %ecx
> >>         shrl    $2, %ecx
> >>  # endif
> >> -       shlx    %SHIFT_REG, %VRDX, %VRDX
> >> -
> >> -# ifdef USE_AS_WCSRCHR
> >> -       kmovw   %edx, %k1
> >> -# else
> >> -       KMOV    %VRDX, %k1
> >> -# endif
> >> -
> >> -       VPCOMPRESS %VMM(6), %VMM(1){%k1}{z}
> >> -       /* We could technically just jmp back after the vpcompress but
> >> -          it doesn't save any 16-byte blocks.  */
> >>         shrx    %SHIFT_REG, %VRSI, %VRSI
> >> +
> >>         test    %VRSI, %VRSI
> >> -       jnz     L(page_cross_return)
> >> -       jmp     L(page_cross_continue)
> >> -       /* 1-byte from cache line.  */
> >> +       jz      L(page_cross_continue)
> >> +
> >> +       /* Found zero CHAR so need to test for search CHAR.  */
> >> +       VPCMP   $0, %VMATCH, %VMM(1), %k1
> >> +       KMOV    %k1, %VRAX
> >> +       /* Shift out search CHAR matches that are before the beginning
> of
> >> +          src (rdi).  */
> >> +       shrx    %SHIFT_REG, %VRAX, %VRAX
> >> +       /* Check if any search CHAR match in range.  */
> >> +       blsmsk  %VRSI, %VRSI
> >> +       and     %VRSI, %VRAX
> >> +       jz      L(ret2)
> >> +       bsr     %VRAX, %VRAX
> >> +#  ifdef USE_AS_WCSRCHR
> >> +       leaq    (%rdi, %rax, CHAR_SIZE), %rax
> >> +#  else
> >> +       addq    %rdi, %rax
> >> +#  endif
> >> +L(ret2):
> >> +       ret
> >
> >
> > Modified instructions to fit page crossing logic in cache line for
> strrchr-evex512.
> >
> >  300: 48 31 f8             xor    %rdi,%rax
> >  303: 62 e1 fe 48 6f 48 3f vmovdqu64 0xfc0(%rax),%zmm17
> >  30a: 62 b2 76 40 26 c1     vptestnmb %zmm17,%zmm17,%k0
> >  310: c4 e1 fb 93 f0       kmovq  %k0,%rsi
> >  315: 89 f9                 mov    %edi,%ecx
> >  317: 48 d3 ee             shr    %cl,%rsi
> >  31a: 0f 84 f8 fc ff ff     je     18 <__strrchr_evex512+0x18>
> >  320: 62 b1 75 40 74 c8     vpcmpeqb %zmm16,%zmm17,%k1
> >  326: c4 e1 fb 93 c1       kmovq  %k1,%rax
> >  32b: 48 d3 e8             shr    %cl,%rax
> >  32e: c4 e2 c8 f3 d6       blsmsk %rsi,%rsi
> >  333: 48 21 f0             and    %rsi,%rax
> >  336: 74 07                 je     33f <__strrchr_evex512+0x33f>
> >  338: 48 0f bd c0           bsr    %rax,%rax
> >  33c: 48 01 f8             add    %rdi,%rax
> >  33f: c3                   ret
> >
> Can update if you want, but its a alot of ifdefs, LMK.
>

I'm not sure why you need to use lots of ifdef. I just modified 4 lines on
top of your latest patch.

diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
index 9cab21c09c..3f4540a041 100644
--- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
+++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
@@ -351,22 +351,20 @@ L(cross_page_boundary):

        /* Shift out zero CHAR matches that are before the beginning of
           src (rdi).  */
-# ifdef USE_AS_WCSRCHR
        movl    %edi, %ecx
+# ifdef USE_AS_WCSRCHR
        andl    $(VEC_SIZE - 1), %ecx
        shrl    $2, %ecx
 # endif
-       shrx    %SHIFT_REG, %VRSI, %VRSI
-
-       test    %VRSI, %VRSI
+       shr     %ecx, %VRSI
        jz      L(page_cross_continue)

        /* Found zero CHAR so need to test for search CHAR.  */
-       VPCMP   $0, %VMATCH, %VMM(1), %k1
+       VPCMPEQ %VMATCH, %VMM(1), %k1
        KMOV    %k1, %VRAX
        /* Shift out search CHAR matches that are before the beginning of
           src (rdi).  */
-       shrx    %SHIFT_REG, %VRAX, %VRAX
+       shr     %ecx, %VRAX
        /* Check if any search CHAR match in range.  */
        blsmsk  %VRSI, %VRSI
        and     %VRSI, %VRAX

At least all string/wcsmbs tests passed.


> >>
> >> +       /* 3 bytes from cache-line for evex.
> >> +          Crosses cache line for evex512.  */
> >>  END(STRRCHR)
> >>  #endif
> >> --
> >> 2.34.1
> >>
>
Noah Goldstein Nov. 12, 2023, 8:01 p.m. UTC | #4
On Sun, Nov 12, 2023 at 11:57 AM Sunil Pandey <skpgkp2@gmail.com> wrote:
>
>
>
> On Fri, Nov 10, 2023 at 7:21 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>>
>> On Thu, Nov 9, 2023 at 9:11 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>> >
>> >
>> >
>> > On Thu, Nov 9, 2023 at 2:49 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>> >>
>> >> strrchr-evex-base used `vpcompress{b|d}` in the page cross logic but
>> >> was missing the CPU_FEATURE checks for VBMI2 in the
>> >> ifunc/ifunc-impl-list.
>> >>
>> >> The fix is either to add those checks or change the logic to not use
>> >> `vpcompress{b|d}`. Choosing the latter here so that the strrchr-evex
>> >> implementation is usable on SKX.
>> >>
>> >> New implementation is a bit slower, but this is in a cold path so its
>> >> probably okay.
>> >> ---
>> >>  sysdeps/x86_64/multiarch/strrchr-evex-base.S | 61 ++++++++++++--------
>> >>  1 file changed, 37 insertions(+), 24 deletions(-)
>> >>
>> >> diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> >> index cd6a0a870a..eb5f1f855a 100644
>> >> --- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> >> +++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> >> @@ -35,7 +35,6 @@
>> >>  #  define CHAR_SIZE    4
>> >>  #  define VPCMP                vpcmpd
>> >>  #  define VPMIN                vpminud
>> >> -#  define VPCOMPRESS   vpcompressd
>> >>  #  define VPTESTN      vptestnmd
>> >>  #  define VPTEST       vptestmd
>> >>  #  define VPBROADCAST  vpbroadcastd
>> >> @@ -46,7 +45,6 @@
>> >>  #  define CHAR_SIZE    1
>> >>  #  define VPCMP                vpcmpb
>> >>  #  define VPMIN                vpminub
>> >> -#  define VPCOMPRESS   vpcompressb
>> >>  #  define VPTESTN      vptestnmb
>> >>  #  define VPTEST       vptestmb
>> >>  #  define VPBROADCAST  vpbroadcastb
>> >> @@ -71,7 +69,7 @@ ENTRY_P2ALIGN(STRRCHR, 6)
>> >>         andl    $(PAGE_SIZE - 1), %eax
>> >>         cmpl    $(PAGE_SIZE - VEC_SIZE), %eax
>> >>         jg      L(cross_page_boundary)
>> >> -
>> >> +L(page_cross_continue):
>> >>         VMOVU   (%rdi), %VMM(1)
>> >>         /* k0 has a 1 for each zero CHAR in YMM1.  */
>> >>         VPTESTN %VMM(1), %VMM(1), %k0
>> >> @@ -79,10 +77,11 @@ ENTRY_P2ALIGN(STRRCHR, 6)
>> >>         test    %VGPR(rsi), %VGPR(rsi)
>> >>         jz      L(aligned_more)
>> >>         /* fallthrough: zero CHAR in first VEC.  */
>> >> -L(page_cross_return):
>> >> +
>> >>         /* K1 has a 1 for each search CHAR match in VEC(1).  */
>> >>         VPCMPEQ %VMATCH, %VMM(1), %k1
>> >>         KMOV    %k1, %VGPR(rax)
>> >> +L(page_cross_return):
>> >
>> >
>> > Unused label.
>> >
>> >>
>> >>         /* Build mask up until first zero CHAR (used to mask of
>> >>            potential search CHAR matches past the end of the string).  */
>> >>         blsmsk  %VGPR(rsi), %VGPR(rsi)
>> >> @@ -167,7 +166,6 @@ L(first_vec_x1_return):
>> >>
>> >>         .p2align 4,, 12
>> >>  L(aligned_more):
>> >> -L(page_cross_continue):
>> >>         /* Need to keep original pointer incase VEC(1) has last match.  */
>> >>         movq    %rdi, %rsi
>> >>         andq    $-VEC_SIZE, %rdi
>> >> @@ -340,34 +338,49 @@ L(return_new_match_ret):
>> >>         leaq    (VEC_SIZE * 2)(%rdi, %rax, CHAR_SIZE), %rax
>> >>         ret
>> >>
>> >> -       .p2align 4,, 4
>> >>  L(cross_page_boundary):
>> >> +       /* eax contains all the page offset bits of src (rdi). `xor rdi,
>> >> +          rax` sets pointer will all page offset bits cleared so
>> >> +          offset of (PAGE_SIZE - VEC_SIZE) will get last aligned VEC
>> >> +          before page cross (guaranteed to be safe to read). Doing this
>> >> +          as opposed to `movq %rdi, %rax; andq $-VEC_SIZE, %rax` saves
>> >> +          a bit of code size.  */
>> >>         xorq    %rdi, %rax
>> >> -       mov     $-1, %VRDX
>> >> -       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(6)
>> >> -       VPTESTN %VMM(6), %VMM(6), %k0
>> >> +       VMOVU   (PAGE_SIZE - VEC_SIZE)(%rax), %VMM(1)
>> >> +       VPTESTN %VMM(1), %VMM(1), %k0
>> >>         KMOV    %k0, %VRSI
>> >>
>> >> +       /* Shift out zero CHAR matches that are before the beginning of
>> >> +          src (rdi).  */
>> >>  # ifdef USE_AS_WCSRCHR
>> >>         movl    %edi, %ecx
>> >> -       and     $(VEC_SIZE - 1), %ecx
>> >> +       andl    $(VEC_SIZE - 1), %ecx
>> >>         shrl    $2, %ecx
>> >>  # endif
>> >> -       shlx    %SHIFT_REG, %VRDX, %VRDX
>> >> -
>> >> -# ifdef USE_AS_WCSRCHR
>> >> -       kmovw   %edx, %k1
>> >> -# else
>> >> -       KMOV    %VRDX, %k1
>> >> -# endif
>> >> -
>> >> -       VPCOMPRESS %VMM(6), %VMM(1){%k1}{z}
>> >> -       /* We could technically just jmp back after the vpcompress but
>> >> -          it doesn't save any 16-byte blocks.  */
>> >>         shrx    %SHIFT_REG, %VRSI, %VRSI
>> >> +
>> >>         test    %VRSI, %VRSI
>> >> -       jnz     L(page_cross_return)
>> >> -       jmp     L(page_cross_continue)
>> >> -       /* 1-byte from cache line.  */
>> >> +       jz      L(page_cross_continue)
>> >> +
>> >> +       /* Found zero CHAR so need to test for search CHAR.  */
>> >> +       VPCMP   $0, %VMATCH, %VMM(1), %k1
>> >> +       KMOV    %k1, %VRAX
>> >> +       /* Shift out search CHAR matches that are before the beginning of
>> >> +          src (rdi).  */
>> >> +       shrx    %SHIFT_REG, %VRAX, %VRAX
>> >> +       /* Check if any search CHAR match in range.  */
>> >> +       blsmsk  %VRSI, %VRSI
>> >> +       and     %VRSI, %VRAX
>> >> +       jz      L(ret2)
>> >> +       bsr     %VRAX, %VRAX
>> >> +#  ifdef USE_AS_WCSRCHR
>> >> +       leaq    (%rdi, %rax, CHAR_SIZE), %rax
>> >> +#  else
>> >> +       addq    %rdi, %rax
>> >> +#  endif
>> >> +L(ret2):
>> >> +       ret
>> >
>> >
>> > Modified instructions to fit page crossing logic in cache line for strrchr-evex512.
>> >
>> >  300: 48 31 f8             xor    %rdi,%rax
>> >  303: 62 e1 fe 48 6f 48 3f vmovdqu64 0xfc0(%rax),%zmm17
>> >  30a: 62 b2 76 40 26 c1     vptestnmb %zmm17,%zmm17,%k0
>> >  310: c4 e1 fb 93 f0       kmovq  %k0,%rsi
>> >  315: 89 f9                 mov    %edi,%ecx
>> >  317: 48 d3 ee             shr    %cl,%rsi
>> >  31a: 0f 84 f8 fc ff ff     je     18 <__strrchr_evex512+0x18>
>> >  320: 62 b1 75 40 74 c8     vpcmpeqb %zmm16,%zmm17,%k1
>> >  326: c4 e1 fb 93 c1       kmovq  %k1,%rax
>> >  32b: 48 d3 e8             shr    %cl,%rax
>> >  32e: c4 e2 c8 f3 d6       blsmsk %rsi,%rsi
>> >  333: 48 21 f0             and    %rsi,%rax
>> >  336: 74 07                 je     33f <__strrchr_evex512+0x33f>
>> >  338: 48 0f bd c0           bsr    %rax,%rax
>> >  33c: 48 01 f8             add    %rdi,%rax
>> >  33f: c3                   ret
>> >
>> Can update if you want, but its a alot of ifdefs, LMK.
>
>
> I'm not sure why you need to use lots of ifdef. I just modified 4 lines on top of your latest patch.
>
> diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> index 9cab21c09c..3f4540a041 100644
> --- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> +++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
> @@ -351,22 +351,20 @@ L(cross_page_boundary):
>
>         /* Shift out zero CHAR matches that are before the beginning of
>            src (rdi).  */
> -# ifdef USE_AS_WCSRCHR
>         movl    %edi, %ecx
> +# ifdef USE_AS_WCSRCHR
>         andl    $(VEC_SIZE - 1), %ecx
>         shrl    $2, %ecx
>  # endif
> -       shrx    %SHIFT_REG, %VRSI, %VRSI
> -
> -       test    %VRSI, %VRSI
> +       shr     %ecx, %VRSI
>         jz      L(page_cross_continue)
>
>         /* Found zero CHAR so need to test for search CHAR.  */
> -       VPCMP   $0, %VMATCH, %VMM(1), %k1
> +       VPCMPEQ %VMATCH, %VMM(1), %k1
>         KMOV    %k1, %VRAX
>         /* Shift out search CHAR matches that are before the beginning of
>            src (rdi).  */
> -       shrx    %SHIFT_REG, %VRAX, %VRAX
> +       shr     %ecx, %VRAX
>         /* Check if any search CHAR match in range.  */
>         blsmsk  %VRSI, %VRSI
>         and     %VRSI, %VRAX
>
> At least all string/wcsmbs tests passed.
>

There is a cost for everything except the avx512 version.
Ill update though.
>>
>> >>
>> >> +       /* 3 bytes from cache-line for evex.
>> >> +          Crosses cache line for evex512.  */
>> >>  END(STRRCHR)
>> >>  #endif
>> >> --
>> >> 2.34.1
>> >>
diff mbox series

Patch

diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
index cd6a0a870a..eb5f1f855a 100644
--- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
+++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
@@ -35,7 +35,6 @@ 
 #  define CHAR_SIZE	4
 #  define VPCMP		vpcmpd
 #  define VPMIN		vpminud
-#  define VPCOMPRESS	vpcompressd
 #  define VPTESTN	vptestnmd
 #  define VPTEST	vptestmd
 #  define VPBROADCAST	vpbroadcastd
@@ -46,7 +45,6 @@ 
 #  define CHAR_SIZE	1
 #  define VPCMP		vpcmpb
 #  define VPMIN		vpminub
-#  define VPCOMPRESS	vpcompressb
 #  define VPTESTN	vptestnmb
 #  define VPTEST	vptestmb
 #  define VPBROADCAST	vpbroadcastb
@@ -71,7 +69,7 @@  ENTRY_P2ALIGN(STRRCHR, 6)
 	andl	$(PAGE_SIZE - 1), %eax
 	cmpl	$(PAGE_SIZE - VEC_SIZE), %eax
 	jg	L(cross_page_boundary)
-
+L(page_cross_continue):
 	VMOVU	(%rdi), %VMM(1)
 	/* k0 has a 1 for each zero CHAR in YMM1.  */
 	VPTESTN	%VMM(1), %VMM(1), %k0
@@ -79,10 +77,11 @@  ENTRY_P2ALIGN(STRRCHR, 6)
 	test	%VGPR(rsi), %VGPR(rsi)
 	jz	L(aligned_more)
 	/* fallthrough: zero CHAR in first VEC.  */
-L(page_cross_return):
+
 	/* K1 has a 1 for each search CHAR match in VEC(1).  */
 	VPCMPEQ	%VMATCH, %VMM(1), %k1
 	KMOV	%k1, %VGPR(rax)
+L(page_cross_return):
 	/* Build mask up until first zero CHAR (used to mask of
 	   potential search CHAR matches past the end of the string).  */
 	blsmsk	%VGPR(rsi), %VGPR(rsi)
@@ -167,7 +166,6 @@  L(first_vec_x1_return):
 
 	.p2align 4,, 12
 L(aligned_more):
-L(page_cross_continue):
 	/* Need to keep original pointer incase VEC(1) has last match.  */
 	movq	%rdi, %rsi
 	andq	$-VEC_SIZE, %rdi
@@ -340,34 +338,49 @@  L(return_new_match_ret):
 	leaq	(VEC_SIZE * 2)(%rdi, %rax, CHAR_SIZE), %rax
 	ret
 
-	.p2align 4,, 4
 L(cross_page_boundary):
+	/* eax contains all the page offset bits of src (rdi). `xor rdi,
+	   rax` sets pointer will all page offset bits cleared so
+	   offset of (PAGE_SIZE - VEC_SIZE) will get last aligned VEC
+	   before page cross (guaranteed to be safe to read). Doing this
+	   as opposed to `movq %rdi, %rax; andq $-VEC_SIZE, %rax` saves
+	   a bit of code size.  */
 	xorq	%rdi, %rax
-	mov	$-1, %VRDX
-	VMOVU	(PAGE_SIZE - VEC_SIZE)(%rax), %VMM(6)
-	VPTESTN	%VMM(6), %VMM(6), %k0
+	VMOVU	(PAGE_SIZE - VEC_SIZE)(%rax), %VMM(1)
+	VPTESTN	%VMM(1), %VMM(1), %k0
 	KMOV	%k0, %VRSI
 
+	/* Shift out zero CHAR matches that are before the beginning of
+	   src (rdi).  */
 # ifdef USE_AS_WCSRCHR
 	movl	%edi, %ecx
-	and	$(VEC_SIZE - 1), %ecx
+	andl	$(VEC_SIZE - 1), %ecx
 	shrl	$2, %ecx
 # endif
-	shlx	%SHIFT_REG, %VRDX, %VRDX
-
-# ifdef USE_AS_WCSRCHR
-	kmovw	%edx, %k1
-# else
-	KMOV	%VRDX, %k1
-# endif
-
-	VPCOMPRESS %VMM(6), %VMM(1){%k1}{z}
-	/* We could technically just jmp back after the vpcompress but
-	   it doesn't save any 16-byte blocks.  */
 	shrx	%SHIFT_REG, %VRSI, %VRSI
+
 	test	%VRSI, %VRSI
-	jnz	L(page_cross_return)
-	jmp	L(page_cross_continue)
-	/* 1-byte from cache line.  */
+	jz	L(page_cross_continue)
+
+	/* Found zero CHAR so need to test for search CHAR.  */
+	VPCMP	$0, %VMATCH, %VMM(1), %k1
+	KMOV	%k1, %VRAX
+	/* Shift out search CHAR matches that are before the beginning of
+	   src (rdi).  */
+	shrx	%SHIFT_REG, %VRAX, %VRAX
+	/* Check if any search CHAR match in range.  */
+	blsmsk	%VRSI, %VRSI
+	and	%VRSI, %VRAX
+	jz	L(ret2)
+	bsr	%VRAX, %VRAX
+#  ifdef USE_AS_WCSRCHR
+	leaq	(%rdi, %rax, CHAR_SIZE), %rax
+#  else
+	addq	%rdi, %rax
+#  endif
+L(ret2):
+	ret
+	/* 3 bytes from cache-line for evex.
+	   Crosses cache line for evex512.  */
 END(STRRCHR)
 #endif