Re: [PATCH v1 1/2] x86: Optimize less_vec evex and avx512 memset-vec-unaligned-erms.S

["H.J. Lu" <hjl.tools@gmail.com>]

On Sun, Apr 18, 2021 at 06:09:21PM -0400, Noah Goldstein wrote:
> No bug. This commit adds optimized cased for less_vec memset case that
> uses the avx512vl/avx512bw mask store avoiding the excessive
> branches. test-memset and test-wmemset are passing.
> 
> Signed-off-by: Noah Goldstein <goldstein.w.n@gmail.com>
> ---
> Tests where run on the following CPUs:
> 
> Icelake: https://ark.intel.com/content/www/us/en/ark/products/196597/intel-core-i7-1065g7-processor-8m-cache-up-to-3-90-ghz.html
> 
> Tigerlake: https://ark.intel.com/content/www/us/en/ark/products/208921/intel-core-i7-1165g7-processor-12m-cache-up-to-4-70-ghz-with-ipu.html
> 
> 
> All times are the geometric mean of N=20. The unit of time is
> seconds.
> 
> "Cur" refers to the current implementation
> "New" refers to this patches implementation
> 
> There are 3 cases that matter for performance.
> 
> 1) ptr is not within VEC_SIZE of a page
> 2) ptr is within VEC_SIZE of a page but length is small enough so that
>    there is not page cross
> 3) page cross.
> 
> Case 1 (which should be the most common) the new implementation has a
> near universal improvement. The only exception is the avx512 case for
> size = [0, 15] where I believe the downclocking from avx512 is causing
> slowdown. Its worth noting that because bench-memset.c repeats the
> same size the branch heavy case should be favored as the branches will
> all be predicted correctly. In a setting with unpredictable length
> this version should perform significant better. For example I
> implemented something similiar to this change for memmove/memcpy and
> saw ~40% speedup in bench-memcpy-random (but for other reasons this
> change isnt good there).
> 
> Cases 2 has a slowdown with this patch (roughly equivilent to the
> performance improvement for case 1). Though I think this is probably
> less important than the improvements for case 1 as page cross are
> probably rarer than non-page cross.
> 
> Case 3 has a very slight slowdown with this patch. But for the same
> reason as above I think this patch is still an improvement.
> 
> Its worth noting that the page cross check could be removed and
> the mask store implementation would still be correct, but I'm finding
> the the fault suppression is incredibly expensive from a performance
> perspective and without the branch I see a 2 orders of magnitude
> performance regression on the Case 2 benchmarks.
> 
> 
...
> 
>  sysdeps/x86_64/multiarch/ifunc-memset.h       |  6 ++-
>  .../multiarch/memset-avx512-unaligned-erms.S  |  2 +-
>  .../multiarch/memset-evex-unaligned-erms.S    |  2 +-
>  .../multiarch/memset-vec-unaligned-erms.S     | 52 +++++++++++++++----
>  4 files changed, 47 insertions(+), 15 deletions(-)
> 
> diff --git a/sysdeps/x86_64/multiarch/ifunc-memset.h b/sysdeps/x86_64/multiarch/ifunc-memset.h
> index 502f946a84..eda5640541 100644
> --- a/sysdeps/x86_64/multiarch/ifunc-memset.h
> +++ b/sysdeps/x86_64/multiarch/ifunc-memset.h
> @@ -54,7 +54,8 @@ IFUNC_SELECTOR (void)
>        && !CPU_FEATURES_ARCH_P (cpu_features, Prefer_No_AVX512))
>      {
>        if (CPU_FEATURE_USABLE_P (cpu_features, AVX512VL)
> -	  && CPU_FEATURE_USABLE_P (cpu_features, AVX512BW))
> +          && CPU_FEATURE_USABLE_P (cpu_features, AVX512BW)
> +          && CPU_FEATURE_USABLE_P (cpu_features, BMI2))
>  	{
>  	  if (CPU_FEATURE_USABLE_P (cpu_features, ERMS))
>  	    return OPTIMIZE (avx512_unaligned_erms);
> @@ -68,7 +69,8 @@ IFUNC_SELECTOR (void)
>    if (CPU_FEATURE_USABLE_P (cpu_features, AVX2))
>      {
>        if (CPU_FEATURE_USABLE_P (cpu_features, AVX512VL)
> -	  && CPU_FEATURE_USABLE_P (cpu_features, AVX512BW))
> +          && CPU_FEATURE_USABLE_P (cpu_features, AVX512BW)
> +          && CPU_FEATURE_USABLE_P (cpu_features, BMI2))
>  	{
>  	  if (CPU_FEATURE_USABLE_P (cpu_features, ERMS))
>  	    return OPTIMIZE (evex_unaligned_erms);

Please also update ifunc-impl-list.c.

> diff --git a/sysdeps/x86_64/multiarch/memset-avx512-unaligned-erms.S b/sysdeps/x86_64/multiarch/memset-avx512-unaligned-erms.S
> index 22e7b187c8..d03460be93 100644
> --- a/sysdeps/x86_64/multiarch/memset-avx512-unaligned-erms.S
> +++ b/sysdeps/x86_64/multiarch/memset-avx512-unaligned-erms.S
> @@ -19,6 +19,6 @@
>  # define SECTION(p)		p##.evex512
>  # define MEMSET_SYMBOL(p,s)	p##_avx512_##s
>  # define WMEMSET_SYMBOL(p,s)	p##_avx512_##s
> -
> +# define USE_LESS_VEC_MASKMOV	1
>  # include "memset-vec-unaligned-erms.S"
>  #endif
> diff --git a/sysdeps/x86_64/multiarch/memset-evex-unaligned-erms.S b/sysdeps/x86_64/multiarch/memset-evex-unaligned-erms.S
> index ae0a4d6e46..eb3541ef60 100644
> --- a/sysdeps/x86_64/multiarch/memset-evex-unaligned-erms.S
> +++ b/sysdeps/x86_64/multiarch/memset-evex-unaligned-erms.S
> @@ -19,6 +19,6 @@
>  # define SECTION(p)		p##.evex
>  # define MEMSET_SYMBOL(p,s)	p##_evex_##s
>  # define WMEMSET_SYMBOL(p,s)	p##_evex_##s
> -
> +# define USE_LESS_VEC_MASKMOV	1
>  # include "memset-vec-unaligned-erms.S"
>  #endif
> diff --git a/sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S b/sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S
> index 584747f1a1..6b02e87f48 100644
> --- a/sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S
> +++ b/sysdeps/x86_64/multiarch/memset-vec-unaligned-erms.S
> @@ -63,6 +63,9 @@
>  # endif
>  #endif
>  
> +#define PAGE_SIZE 4096
> +#define LOG_PAGE_SIZE 12
> +
>  #ifndef SECTION
>  # error SECTION is not defined!
>  #endif
> @@ -213,11 +216,38 @@ L(loop):
>  	cmpq	%rcx, %rdx
>  	jne	L(loop)
>  	VZEROUPPER_SHORT_RETURN
> +
> +	.p2align 4
>  L(less_vec):
>  	/* Less than 1 VEC.  */
>  # if VEC_SIZE != 16 && VEC_SIZE != 32 && VEC_SIZE != 64
>  #  error Unsupported VEC_SIZE!
>  # endif
> +# ifdef USE_LESS_VEC_MASKMOV
> +	/* Clear high bits from edi. Only keeping bits relevant to page
> +	   cross check. Using sall instead of andl saves 3 bytes. Note
> +	   that we are using rax which is set in
> +	   MEMSET_VDUP_TO_VEC0_AND_SET_RETURN as ptr from here on out.  */
> +	sall	$(32 - LOG_PAGE_SIZE), %edi

Why is SAL needed here?

> +	/* Check if VEC_SIZE load cross page. Mask loads suffer serious
> +	   performance degradation when it has to fault supress.  */
> +	cmpl	$((PAGE_SIZE - VEC_SIZE) << (32 - LOG_PAGE_SIZE)), %edi

Can you use only CMP, not SAL, for masked store?

> +	ja	L(cross_page)
> +# if VEC_SIZE > 32
> +	movq	$-1, %rcx
> +	bzhiq	%rdx, %rcx, %rcx
> +	kmovq	%rcx, %k1
> +# else
> +	movl	$-1, %ecx
> +	bzhil	%edx, %ecx, %ecx
> +	kmovd	%ecx, %k1
> +# endif
> +	vmovdqu8	%VEC(0), (%rax) {%k1}
> +	VZEROUPPER_RETURN
> +
> +	.p2align 4
> +L(cross_page):
> +# endif
>  # if VEC_SIZE > 32
>  	cmpb	$32, %dl
>  	jae	L(between_32_63)
> @@ -234,36 +264,36 @@ L(less_vec):
>  	cmpb	$1, %dl
>  	ja	L(between_2_3)
>  	jb	1f
> -	movb	%cl, (%rdi)
> +	movb	%cl, (%rax)
>  1:
>  	VZEROUPPER_RETURN
>  # if VEC_SIZE > 32
>  	/* From 32 to 63.  No branch when size == 32.  */
>  L(between_32_63):
> -	VMOVU	%YMM0, -32(%rdi,%rdx)
> -	VMOVU	%YMM0, (%rdi)
> +	VMOVU	%YMM0, -32(%rax,%rdx)
> +	VMOVU	%YMM0, (%rax)
>  	VZEROUPPER_RETURN
>  # endif
>  # if VEC_SIZE > 16
>  	/* From 16 to 31.  No branch when size == 16.  */
>  L(between_16_31):
> -	VMOVU	%XMM0, -16(%rdi,%rdx)
> -	VMOVU	%XMM0, (%rdi)
> +	VMOVU	%XMM0, -16(%rax,%rdx)
> +	VMOVU	%XMM0, (%rax)
>  	VZEROUPPER_RETURN
>  # endif
>  	/* From 8 to 15.  No branch when size == 8.  */
>  L(between_8_15):
> -	movq	%rcx, -8(%rdi,%rdx)
> -	movq	%rcx, (%rdi)
> +	movq	%rcx, -8(%rax,%rdx)
> +	movq	%rcx, (%rax)
>  	VZEROUPPER_RETURN
>  L(between_4_7):
>  	/* From 4 to 7.  No branch when size == 4.  */
> -	movl	%ecx, -4(%rdi,%rdx)
> -	movl	%ecx, (%rdi)
> +	movl	%ecx, -4(%rax,%rdx)
> +	movl	%ecx, (%rax)
>  	VZEROUPPER_RETURN
>  L(between_2_3):
>  	/* From 2 to 3.  No branch when size == 2.  */
> -	movw	%cx, -2(%rdi,%rdx)
> -	movw	%cx, (%rdi)
> +	movw	%cx, -2(%rax,%rdx)
> +	movw	%cx, (%rax)
>  	VZEROUPPER_RETURN
>  END (MEMSET_SYMBOL (__memset, unaligned_erms))
> -- 
> 2.29.2
> 

H.J.