Re: [PATCH ver 3] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation

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

Hi Carl,

on 2023/8/8 01:50, Carl Love wrote:
> 
> GCC maintainers:
> 
> Ver 3: Updated description to make it clear the patch fixes the
> confusion on the availability of the builtins.  Fixed the dg-require-
> effective-target on the test cases and the dg-options.  Change the test
> case so the for loop for the test will not be unrolled.  Fixed a
> spelling error in a vec-cmpne.c comment.  Retested on Power 10LE.
> 
> Ver 2:  Re-worked the test vec-cmpne.c to create a compile only test
> verify the instruction generation and a runnable test to verify the
> built-in functionality.  Retested the patch on Power 8 LE/BE, Power
> 9LE/BE and Power 10 LE with no regressions.
> 
> The following patch cleans up the definition for the
> __builtin_altivec_vcmpne{b,h,w}.  The current implementation implies
> that the built-in is only supported on Power 9 since it is defined
> under the Power 9 stanza.  However the built-in has no ISA restrictions
> as stated in the Power Vector Intrinsic Programming Reference document.
> The current built-in works because the built-in gets replaced during
> GIMPLE folding by a simple not-equal operator so it doesn't get
> expanded and checked for Power 9 code generation.
> 
> This patch moves the definition to the Altivec stanza in the built-in
> definition file to make it clear the built-ins are valid for Power 8,
> Power 9 and beyond.  
> 
> The patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10
> LE with no regressions.
> 
> Please let me know if the patch is acceptable for mainline.  Thanks.
> 
>                       Carl 
> 
> ------------------------------------------------
> 
> rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
> 
> The current built-in definitions for vcmpneb, vcmpneh, vcmpnew are defined
> under the Power 9 section of r66000-builtins.  This implies they are only
> supported on Power 9 and above when in fact they are defined and work with
> Altivec as well with the appropriate Altivec instruction generation.
> 
> The vec_cmpne builtin should generate the vcmpequ{b,h,w} instruction with
> Altivec enabled and generate the vcmpne{b,h,w} on Power 9 and newer
> processors.
> 
> This patch moves the definitions to the Altivec stanza to make it clear
> the built-ins are supported for all Altivec processors.  The patch
> removes the confusion as to which processors support the vcmpequ{b,h,w}
> instructions.
> 
> There is existing test coverage for the vec_cmpne built-in for
> vector bool char, vector bool short, vector bool int,
> vector bool long long in builtins-3-p9.c and p8vector-builtin-2.c.
> Coverage for vector signed int, vector unsigned int is in
> p8vector-builtin-2.c.
> 
> Test vec-cmpne.c is updated to check the generation of the vcmpequ{b,h,w}
> instructions for Altivec.  A new test vec-cmpne-runnable.c is added to
> verify the built-ins work as expected.
> 
> Patch has been tested on Power 8 LE/BE, Power 9 LE/BE and Power 10 LE
> with no regressions.

Okay for trunk with two nits below fixed, thanks!

> 
> gcc/ChangeLog:
> 
> 	* config/rs6000/rs6000-builtins.def (vcmpneb, vcmpneh, vcmpnew):
> 	Move definitions to Altivec stanza.
> 	* config/rs6000/altivec.md (vcmpneb, vcmpneh, vcmpnew): New
> 	define_expand.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/vec-cmpne-runnable.c: New execution test.
> 	* gcc.target/powerpc/vec-cmpne.c (define_test_functions,
> 	execute_test_functions) moved to vec-cmpne.h.  Added
> 	scan-assembler-times for vcmpequb, vcmpequh, vcmpequw.

	s/ moved/: Move/ => "... execute_test_functions): Move "
	
        s/Added/Add/

> 	* gcc.target/powerpc/vec-cmpne.h: New include file for vec-cmpne.c
> 	and vec-cmpne-runnable.c. Split define_test_functions definition
> 	into define_test_functions and define_init_verify_functions.
> ---
>  gcc/config/rs6000/altivec.md                  |  12 ++
>  gcc/config/rs6000/rs6000-builtins.def         |  18 +--
>  .../gcc.target/powerpc/vec-cmpne-runnable.c   |  36 ++++++
>  gcc/testsuite/gcc.target/powerpc/vec-cmpne.c  | 112 ++----------------
>  gcc/testsuite/gcc.target/powerpc/vec-cmpne.h  |  90 ++++++++++++++
>  5 files changed, 156 insertions(+), 112 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vec-cmpne-runnable.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/vec-cmpne.h
> 
> diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
> index ad1224e0b57..31f65aa1b7a 100644
> --- a/gcc/config/rs6000/altivec.md
> +++ b/gcc/config/rs6000/altivec.md
> @@ -2631,6 +2631,18 @@ (define_insn "altivec_vcmpequt_p"
>    "vcmpequq. %0,%1,%2"
>    [(set_attr "type" "veccmpfx")])
>  
> +;; Expand for builtin vcmpne{b,h,w}
> +(define_expand "altivec_vcmpne_<mode>"
> +  [(set (match_operand:VSX_EXTRACT_I 3 "altivec_register_operand" "=v")
> +	(eq:VSX_EXTRACT_I (match_operand:VSX_EXTRACT_I 1 "altivec_register_operand" "v")
> +			  (match_operand:VSX_EXTRACT_I 2 "altivec_register_operand" "v")))
> +   (set (match_operand:VSX_EXTRACT_I 0 "altivec_register_operand" "=v")
> +        (not:VSX_EXTRACT_I (match_dup 3)))]
> +  "TARGET_ALTIVEC"
> +  {
> +    operands[3] = gen_reg_rtx (GET_MODE (operands[0]));
> +  });

Nit: Useless ";".

BR,
Kewen