RE: [PATCH] eal/x86: optimize memcpy of small 64-byte blocks

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> The implementation for copying 64-byte blocks up to 512 (or 256) bytes
> does not depend on address alignment with the size of the CPU's vector
> registers, but is implemented in both unaligned and aligned copy
> functions.
> The main rte_memcpy() function was updated, so
> if the copy size is known at compile time and the other criteria match,
> the copy is performed without checking alignment requirements.
> This provides two benefits when the optimization comes into play:
> 1. A performance gain, because the address alignment check is avoided.
> 3. Reduced instruction memory footprint, because the compiler only
> generates one instance of the function for copying, instead of two
> instances (one in the unaligned copy function, and one in the aligned
> copy function).
> 
> Furthermore, the temporary alignment mask definition (ALIGNMENT_MASK)
> was prefixed by RTE_MEMCPY_ to prevent potential namespace collision.
> 
> And finally, the superfluous function declaration at the top of the
> file was removed, and its description was moved to the function
> definition.
> This improves search results with source code browsers.
> 
> Signed-off-by: Morten Brørup <mb@smartsharesystems.com>
> ---
>  lib/eal/x86/include/rte_memcpy.h | 64 ++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/eal/x86/include/rte_memcpy.h
> b/lib/eal/x86/include/rte_memcpy.h
> index 8ed8c55010..ce5a55c36f 100644
> --- a/lib/eal/x86/include/rte_memcpy.h
> +++ b/lib/eal/x86/include/rte_memcpy.h
> @@ -32,21 +32,6 @@ extern "C" {
>  #define RTE_MEMCPY_AVX
>  #endif
> 
> -/**
> - * Copy bytes from one location to another. The locations must not overlap.
> - *
> - * @param dst
> - *   Pointer to the destination of the data.
> - * @param src
> - *   Pointer to the source data.
> - * @param n
> - *   Number of bytes to copy.
> - * @return
> - *   Pointer to the destination data.
> - */
> -static __rte_always_inline void *
> -rte_memcpy(void *__rte_restrict dst, const void *__rte_restrict src, size_t n);
> -
>  /**
>   * Copy bytes from one location to another,
>   * locations must not overlap.
> @@ -187,7 +172,8 @@ rte_mov256(uint8_t *__rte_restrict dst, const uint8_t
> *__rte_restrict src)
>   * AVX512 implementation below
>   */
> 
> -#define ALIGNMENT_MASK 0x3F
> +#define RTE_MEMCPY_ALIGNMENT_MASK 0x3F
> +#define RTE_MEMCPY_BLOCK_64_MAX 512
> 
>  /**
>   * Copy 128-byte blocks from one location to another,
> @@ -333,7 +319,8 @@ rte_memcpy_generic_more_than_64(void *__rte_restrict
> dst, const void *__rte_rest
>   * AVX implementation below
>   */
> 
> -#define ALIGNMENT_MASK 0x1F
> +#define RTE_MEMCPY_ALIGNMENT_MASK 0x1F
> +#define RTE_MEMCPY_BLOCK_64_MAX 256

Wonder why BLOCK_64_MAX is 512 for SSE and AVX512, but 256 for AVX2?
Some empirical data or ...?

> 
>  /**
>   * Copy 128-byte blocks from one location to another,
> @@ -444,7 +431,8 @@ rte_memcpy_generic_more_than_64(void *__rte_restrict
> dst, const void *__rte_rest
>   * SSE implementation below
>   */
> 
> -#define ALIGNMENT_MASK 0x0F
> +#define RTE_MEMCPY_ALIGNMENT_MASK 0x0F
> +#define RTE_MEMCPY_BLOCK_64_MAX 512
> 
>  /**
>   * Macro for copying unaligned block from one location to another with
> constant load offset,
> @@ -673,6 +661,18 @@ rte_memcpy_aligned_more_than_64(void
> *__rte_restrict dst, const void *__rte_rest
>  	return ret;
>  }
> 
> +/**
> + * Copy bytes from one location to another. The locations must not overlap.
> + *
> + * @param dst
> + *   Pointer to the destination of the data.
> + * @param src
> + *   Pointer to the source data.
> + * @param n
> + *   Number of bytes to copy.
> + * @return
> + *   Pointer to the destination data.
> + */
>  static __rte_always_inline void *
>  rte_memcpy(void *__rte_restrict dst, const void *__rte_restrict src, size_t n)
>  {
> @@ -707,15 +707,39 @@ rte_memcpy(void *__rte_restrict dst, const void
> *__rte_restrict src, size_t n)
>  #endif
>  		return dst;
>  	}
> +	/* Common way for small copy size of 64-byte blocks. Unlikely, so
> constant size only */
> +	if (__rte_constant(n) && (n & 63) == 0 && n <=
> RTE_MEMCPY_BLOCK_64_MAX) {

You probably need a check (static_assert?) that RTE_MEMCPY_BLOCK_64_MAX is 
Within allowed values (<= 512+256+128+64).
Otherwise code block below might not be enough to copy everything.

> +		void *ret = dst;
> +
> +		if (n & 512) {
> +			rte_mov256((uint8_t *)dst + 0 * 256, (const uint8_t *)src
> + 0 * 256);
> +			rte_mov256((uint8_t *)dst + 1 * 256, (const uint8_t *)src
> + 1 * 256);
> +		}
> +		if (n & 256) {
> +			rte_mov256((uint8_t *)dst, (const uint8_t *)src);
> +			src = (const uint8_t *)src + 256;
> +			dst = (uint8_t *)dst + 256;
> +		}
> +		if (n & 128) {
> +			rte_mov128((uint8_t *)dst, (const uint8_t *)src);
> +			src = (const uint8_t *)src + 128;
> +			dst = (uint8_t *)dst + 128;
> +		}
> +		if (n & 64)
> +			rte_mov64((uint8_t *)dst, (const uint8_t *)src);
> +
> +		return ret;
> +	}
> 
>  	/* Implementation for size > 64 bytes depends on alignment with vector
> register size. */
> -	if (!(((uintptr_t)dst | (uintptr_t)src) & ALIGNMENT_MASK))
> +	if (!(((uintptr_t)dst | (uintptr_t)src) & RTE_MEMCPY_ALIGNMENT_MASK))
>  		return rte_memcpy_aligned_more_than_64(dst, src, n);
>  	else
>  		return rte_memcpy_generic_more_than_64(dst, src, n);
>  }
> 
> -#undef ALIGNMENT_MASK
> +#undef RTE_MEMCPY_ALIGNMENT_MASK
> +#undef RTE_MEMCPY_BLOCK_64_MAX
> 
>  #ifdef __cplusplus
>  }
> --
> 2.43.0