Re: [Patchv2, rs6000] Clean up pre-checkings of expand_block_compare

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Haochen,

on 2023/12/18 10:44, HAO CHEN GUI wrote:
> Hi,
>   This patch cleans up pre-checkings of expand_block_compare. It does
> 1. Assert only P7 above can enter this function as it's already guard
> by the expand.
> 2. Return false when optimizing for size.
> 3. Remove P7 processor test as only P7 above can enter this function and
> P7 LE is excluded by targetm.slow_unaligned_access. On P7 BE, the
> performance of expand is better than the performance of library when
> the length is long.

Maybe it's better to split the handling for optimizing for size out to a
separated patch, since it's not actually a clean up.  Sorry, I should
have suggested this in the previous review.  For 3, as you have evaluated
the performance on Power7, I think it's safe to make this change now, so
this patch is ok for trunk, thanks!

> 
>   Compared to last version,
> https://gcc.gnu.org/pipermail/gcc-patches/2023-December/640082.html
> the main change is to add some comments and move the variable definition
> closed to its use.
> 
>   Bootstrapped and tested on x86 and powerpc64-linux BE and LE with no
> regressions. Is this OK for trunk?
> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> rs6000: Clean up the pre-checkings of expand_block_compare
> 
> gcc/
> 	* gcc/config/rs6000/rs6000-string.cc (expand_block_compare): Assert
> 	only P7 above can enter this function.  Return false (call library)
> 	when it's optimized for size.  Remove P7 CPU test as only P7 above
> 	can enter this function and P7 LE is excluded by the checking of
> 	targetm.slow_unaligned_access on word_mode.  Also performance test
> 	shows the expand of block compare with 16 bytes to 64 bytes length
> 	is better than library on P7 BE.

Nit: You can just describe "what's done" but not "why" here, and put "why"
into the commit log instead.

BR,
Kewen

> 
> gcc/testsuite/
> 	* gcc.target/powerpc/block-cmp-3.c: New.
> 
> 
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000-string.cc b/gcc/config/rs6000/rs6000-string.cc
> index cb9eeef05d8..49670cef4d7 100644
> --- a/gcc/config/rs6000/rs6000-string.cc
> +++ b/gcc/config/rs6000/rs6000-string.cc
> @@ -1946,36 +1946,32 @@ expand_block_compare_gpr(unsigned HOST_WIDE_INT bytes, unsigned int base_align,
>  bool
>  expand_block_compare (rtx operands[])
>  {
> -  rtx target = operands[0];
> -  rtx orig_src1 = operands[1];
> -  rtx orig_src2 = operands[2];
> -  rtx bytes_rtx = operands[3];
> -  rtx align_rtx = operands[4];
> +  /* TARGET_POPCNTD is already guarded at expand cmpmemsi.  */
> +  gcc_assert (TARGET_POPCNTD);
> 
> -  /* This case is complicated to handle because the subtract
> -     with carry instructions do not generate the 64-bit
> -     carry and so we must emit code to calculate it ourselves.
> -     We choose not to implement this yet.  */
> -  if (TARGET_32BIT && TARGET_POWERPC64)
> +  if (optimize_insn_for_size_p ())
>      return false;
> 
> -  bool isP7 = (rs6000_tune == PROCESSOR_POWER7);
> -
>    /* Allow this param to shut off all expansion.  */
>    if (rs6000_block_compare_inline_limit == 0)
>      return false;
> 
> -  /* targetm.slow_unaligned_access -- don't do unaligned stuff.
> -     However slow_unaligned_access returns true on P7 even though the
> -     performance of this code is good there.  */
> -  if (!isP7
> -      && (targetm.slow_unaligned_access (word_mode, MEM_ALIGN (orig_src1))
> -	  || targetm.slow_unaligned_access (word_mode, MEM_ALIGN (orig_src2))))
> +  /* This case is complicated to handle because the subtract
> +     with carry instructions do not generate the 64-bit
> +     carry and so we must emit code to calculate it ourselves.
> +     We choose not to implement this yet.  */
> +  if (TARGET_32BIT && TARGET_POWERPC64)
>      return false;
> 
> -  /* Unaligned l*brx traps on P7 so don't do this.  However this should
> -     not affect much because LE isn't really supported on P7 anyway.  */
> -  if (isP7 && !BYTES_BIG_ENDIAN)
> +  rtx target = operands[0];
> +  rtx orig_src1 = operands[1];
> +  rtx orig_src2 = operands[2];
> +  rtx bytes_rtx = operands[3];
> +  rtx align_rtx = operands[4];
> +
> +  /* targetm.slow_unaligned_access -- don't do unaligned stuff.  */
> +    if (targetm.slow_unaligned_access (word_mode, MEM_ALIGN (orig_src1))
> +	|| targetm.slow_unaligned_access (word_mode, MEM_ALIGN (orig_src2)))
>      return false;
> 
>    /* If this is not a fixed size compare, try generating loop code and
> @@ -2023,14 +2019,6 @@ expand_block_compare (rtx operands[])
>    if (!IN_RANGE (bytes, 1, max_bytes))
>      return expand_compare_loop (operands);
> 
> -  /* The code generated for p7 and older is not faster than glibc
> -     memcmp if alignment is small and length is not short, so bail
> -     out to avoid those conditions.  */
> -  if (targetm.slow_unaligned_access (word_mode, UINTVAL (align_rtx))
> -      && ((base_align == 1 && bytes > 16)
> -	  || (base_align == 2 && bytes > 32)))
> -    return false;
> -
>    rtx final_label = NULL;
> 
>    if (use_vec)
> diff --git a/gcc/testsuite/gcc.target/powerpc/block-cmp-3.c b/gcc/testsuite/gcc.target/powerpc/block-cmp-3.c
> new file mode 100644
> index 00000000000..c7e853ad593
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/block-cmp-3.c
> @@ -0,0 +1,8 @@
> +/* { dg-do compile } */
> +/* { dg-options "-Os" } */
> +/* { dg-final { scan-assembler-times {\mb[l]? memcmp\M} 1 } }  */
> +
> +int foo (const char* s1, const char* s2)
> +{
> +  return __builtin_memcmp (s1, s2, 4);
> +}