Re: [PATCH ver 3] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
Kewen:
On Wed, 2023-08-09 at 16:47 +0800, Kewen.Lin wrote:
> > 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!
Thanks for all the help with the patch. Fixed the nits below, compiled
and reran the test cases to make sure everything was OK. Will go ahead
and commit the patch.
>
> > 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/
Fixed both issues.
>
> >
> > +;; Expand for builtin vcmpne{b,h,w}
> > +(define_expand "altivec_vcmpne_"
> > + [(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 ";".
removed semicolon.
Carl
Re: [PATCH ver 3] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
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_"
> + [(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")
> +
[PATCH ver 3] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
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.
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.
* 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_"
+ [(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]));
+ });
+
(define_insn "*altivec_vcmpgts_p"
[(set (reg:CC CR6_REGNO)
(unspec:CC [(gt:CC (match_operand:VI2 1
