From: Noah Goldstein <goldstein.w.n@gmail.com>
To: Sunil Pandey <skpgkp2@gmail.com>
Cc: libc-alpha@sourceware.org, hjl.tools@gmail.com, carlos@systemhalted.org
Subject: Re: x86: Fix unchecked AVX512-VBMI2 usage in strrchr-evex-base.S
Date: Wed, 8 Nov 2023 15:33:03 -0600 [thread overview]
Message-ID: <CAFUsyfJiVy7PzNVCPCbzhoeV2TmC604xXOMNaULr-yEPWvqu+g@mail.gmail.com> (raw)
In-Reply-To: <CAMAf5_fYj7ut0u8Y=9uYjs3LHhODFTSgUzYYDTghUBUgqvQZUg@mail.gmail.com>
On Wed, Nov 8, 2023 at 1:05 PM Sunil Pandey <skpgkp2@gmail.com> wrote:
>
>
>
> On Wed, Nov 1, 2023 at 3:17 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 | 64 +++++++++++++-------
>> 1 file changed, 43 insertions(+), 21 deletions(-)
>>
>> diff --git a/sysdeps/x86_64/multiarch/strrchr-evex-base.S b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> index cd6a0a870a..b174d43208 100644
>> --- a/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> +++ b/sysdeps/x86_64/multiarch/strrchr-evex-base.S
>> @@ -71,7 +71,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 +79,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 +168,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 +340,56 @@ 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
>> + shrx %SHIFT_REG, %VRSI, %VRSI
>>
>> -# ifdef USE_AS_WCSRCHR
>> - kmovw %edx, %k1
>> + test %VRSI, %VRSI
>> + 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
>> + /* For VEC_SIZE == 64 we are just at the end of a cacheline here
>> + so to save code-size just re-use return logic for first VEC.
>> + This is relatively cold code (page cross). */
>> +# if VEC_SIZE == 64
>> + jmp L(page_cross_return)
>
>
> Can you please remove unconditional jump. It's simply confusing and doesn't help across the board.
done for wcsrchr-evex512
>
> $ tail -7 /tmp/wcsrchr-evex.out
> 301: c4 e2 73 f7 c0 shrx %ecx,%eax,%eax
> 306: c4 e2 48 f3 d6 blsmsk %esi,%esi
> 30b: 21 f0 and %esi,%eax
> 30d: 74 07 je 316 <__wcsrchr_evex+0x316>
> 30f: 0f bd c0 bsr %eax,%eax
> 312: 48 8d 04 87 lea (%rdi,%rax,4),%rax
> 316: c3 ret
>
> $ tail -7 /tmp/wcsrchr-evex512.out
> 300: c4 e2 73 f7 f6 shrx %ecx,%esi,%esi
> 305: 85 f6 test %esi,%esi
> 307: 0f 84 0b fd ff ff je 18 <__wcsrchr_evex512+0x18>
> 30d: 62 b3 75 40 1f c8 00 vpcmpeqd %zmm16,%zmm17,%k1
> 314: c5 fb 93 c1 kmovd %k1,%eax
> 318: c4 e2 73 f7 c0 shrx %ecx,%eax,%eax
> 31d: e9 18 fd ff ff jmp 3a <__wcsrchr_evex512+0x3a>
>
> $ tail -7 /tmp/strrchr-evex.out
> 2e8: c4 e2 43 f7 c0 shrx %edi,%eax,%eax
> 2ed: c4 e2 48 f3 d6 blsmsk %esi,%esi
> 2f2: 21 f0 and %esi,%eax
> 2f4: 74 06 je 2fc <__strrchr_evex+0x2fc>
> 2f6: 0f bd c0 bsr %eax,%eax
> 2f9: 48 01 f8 add %rdi,%rax
> 2fc: c3 ret
>
> $ tail -7 /tmp/strrchr-evex512.out
> 315: c4 e2 c3 f7 f6 shrx %rdi,%rsi,%rsi
> 31a: 48 85 f6 test %rsi,%rsi
> 31d: 0f 84 f5 fc ff ff je 18 <__strrchr_evex512+0x18>
> 323: 62 b3 75 40 3f c8 00 vpcmpeqb %zmm16,%zmm17,%k1
> 32a: c4 e1 fb 93 c1 kmovq %k1,%rax
> 32f: c4 e2 c3 f7 c0 shrx %rdi,%rax,%rax
> 334: e9 04 fd ff ff jmp 3d <__strrchr_evex512+0x3d>
>
>
>>
>> + /* 6 bytes from cache-line. */
>> # else
>> - KMOV %VRDX, %k1
>> + /* 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. */
>> # endif
>> -
>> - VPCOMPRESS %VMM(6), %VMM(1){%k1}{z}
>
>
> Please remove the VCOMPRESS definition as it's not used.
done.
>
>>
>> - /* 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. */
>> END(STRRCHR)
>> #endif
>> --
>> 2.34.1
>>
next prev parent reply other threads:[~2023-11-08 21:33 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-01 22:16 Noah Goldstein
2023-11-07 17:51 ` Noah Goldstein
2023-11-07 18:25 ` Florian Weimer
2023-11-08 5:22 ` Sunil Pandey
2023-11-08 19:04 ` Sunil Pandey
2023-11-08 19:22 ` Noah Goldstein
2023-11-08 20:08 ` Sunil Pandey
2023-11-08 21:21 ` Noah Goldstein
2023-11-08 21:22 ` Noah Goldstein
2023-11-08 21:33 ` Noah Goldstein [this message]
2023-11-08 21:33 ` [PATCH v2] " Noah Goldstein
2023-11-09 0:10 ` Sunil Pandey
2023-11-09 1:51 ` Noah Goldstein
2023-11-09 22:49 ` [PATCH v3] " Noah Goldstein
2023-11-10 3:10 ` Sunil Pandey
2023-11-11 3:21 ` Noah Goldstein
2023-11-12 17:56 ` Sunil Pandey
2023-11-12 20:01 ` Noah Goldstein
2023-11-11 3:20 ` [PATCH v4] " Noah Goldstein
2023-11-12 20:51 ` Noah Goldstein
2023-11-13 14:07 ` Sunil Pandey
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFUsyfJiVy7PzNVCPCbzhoeV2TmC604xXOMNaULr-yEPWvqu+g@mail.gmail.com \
--to=goldstein.w.n@gmail.com \
--cc=carlos@systemhalted.org \
--cc=hjl.tools@gmail.com \
--cc=libc-alpha@sourceware.org \
--cc=skpgkp2@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).