public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Noah Goldstein <goldstein.w.n@gmail.com>
To: libc-alpha@sourceware.org
Cc: goldstein.w.n@gmail.com, hjl.tools@gmail.com, carlos@systemhalted.org
Subject: [PATCH v4] x86: Fix unchecked AVX512-VBMI2 usage in strrchr-evex-base.S
Date: Fri, 10 Nov 2023 21:20:47 -0600	[thread overview]
Message-ID: <20231111032047.2138614-1-goldstein.w.n@gmail.com> (raw)
In-Reply-To: <20231101221657.311121-1-goldstein.w.n@gmail.com>

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 | 60 ++++++++++++--------
 1 file changed, 36 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..9cab21c09c 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,7 +77,7 @@ 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)
@@ -167,7 +165,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 +337,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
-- 
2.34.1


  parent reply	other threads:[~2023-11-11  3:20 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
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 ` Noah Goldstein [this message]
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=20231111032047.2138614-1-goldstein.w.n@gmail.com \
    --to=goldstein.w.n@gmail.com \
    --cc=carlos@systemhalted.org \
    --cc=hjl.tools@gmail.com \
    --cc=libc-alpha@sourceware.org \
    /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).