Re: [PATCH] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
Kewen:
On Mon, 2023-07-31 at 14:53 +0800, Kewen.Lin wrote:
> Hi Carl,
>
> on 2023/7/28 23:00, Carl Love wrote:
> > GCC maintainers:
> >
> > The following patch cleans up the definition for the
> > __builtin_altivec_vcmpnet. The current implementation implies that
> > the
>
> s/__builtin_altivec_vcmpnet/__builtin_altivec_vcmpne[bhw]/
OK, updated in email for version 2.
>
> > 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 on
> > Power 8 as well with the appropriate Power 8 instruction
> > generation.
>
> Nit: It's confusing to say Power8 only, it's actually supported once
> altivec
> is enabled, so I think it's more clear to replace Power8 with altivec
> here.
OK, replaced Power 8 with Altivec here and for additional instances of
Power 8 below.
>
> > The vec_cmpne builtin should generate the vcmpequ{b,h,w}
> > instruction on
> > Power 8 and generate the vcmpne{b,h,w} on Power 9 an newer
> > processors.
>
>
> Ditto for Power8 and "an" -> "and"?
Fixed, fixed.
>
> > This patch moves the definitions to the Altivec stanza to make it
> > clear
> > the built-ins are supported for all Altivec processors. The patch
> > enables the vcmpequ{b,h,w} instruction to be generated on Power 8
> > and
> > the vcmpne{b,h,w} instruction to be generated on Power 9 and
> > beyond.
>
> Ditto for Power8.
fixed
>
> > 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.
>
> So there is no coverage with the basic altivec support. I noticed
> we have one test case "gcc/testsuite/gcc.target/powerpc/vec-cmpne.c"
> which is a test case for running but with vsx_ok, I think we can
> rewrite it with altivec (vmx), either separating to compiling and
> running case, or adding -save-temp and check expected insns.
I looked at just adding -save-temp and scan-assembler-times for the
instructions. I noticed that vcmpequw occurs 30 times in the functions
to initialize and test the results. So, I opted to create a separate
compile/check instructions test and a runnable test to verify the
functionality. This way any changes in the code to calculate and
verify the results will not break the instruction generation checks.
>
> Coverage for unsigned long long int and long long int
> > for Power 10 in int_128bit-runnable.c.
Removed comment about Power 10, long long int testing.
> >
> > 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.
> > vcmpnet): Move definitions to Altivec stanza.
>
> vcmpnet which isn't handled in this patch should be removed.
Removed.
Carl
Re: [PATCH] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
Hi Carl,
on 2023/7/28 23:00, Carl Love wrote:
> GCC maintainers:
>
> The following patch cleans up the definition for the
> __builtin_altivec_vcmpnet. The current implementation implies that the
s/__builtin_altivec_vcmpnet/__builtin_altivec_vcmpne[bhw]/
> 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 on
> Power 8 as well with the appropriate Power 8 instruction generation.
Nit: It's confusing to say Power8 only, it's actually supported once altivec
is enabled, so I think it's more clear to replace Power8 with altivec here.
>
> The vec_cmpne builtin should generate the vcmpequ{b,h,w} instruction on
> Power 8 and generate the vcmpne{b,h,w} on Power 9 an newer processors.
Ditto for Power8 and "an" -> "and"?
>
> This patch moves the definitions to the Altivec stanza to make it clear
> the built-ins are supported for all Altivec processors. The patch
> enables the vcmpequ{b,h,w} instruction to be generated on Power 8 and
> the vcmpne{b,h,w} instruction to be generated on Power 9 and beyond.
Ditto for Power8.
>
> 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.
So there is no coverage with the basic altivec support. I noticed
we have one test case "gcc/testsuite/gcc.target/powerpc/vec-cmpne.c"
which is a test case for running but with vsx_ok, I think we can
rewrite it with altivec (vmx), either separating to compiling and
running case, or adding -save-temp and check expected insns.
Coverage for unsigned long long int and long long int
> for Power 10 in int_128bit-runnable.c.
>
> 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.
> vcmpnet): Move definitions to Altivec stanza.
vcmpnet which isn't handled in this patch should be removed.
The others look good to me, thanks!
BR,
Kewen
[PATCH] rs6000: Fix __builtin_altivec_vcmpne{b,h,w} implementation
GCC maintainers:
The following patch cleans up the definition for the
__builtin_altivec_vcmpnet. 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 on
Power 8 as well with the appropriate Power 8 instruction generation.
The vec_cmpne builtin should generate the vcmpequ{b,h,w} instruction on
Power 8 and generate the vcmpne{b,h,w} on Power 9 an 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
enables the vcmpequ{b,h,w} instruction to be generated on Power 8 and
the vcmpne{b,h,w} instruction to be generated on Power 9 and beyond.
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. Coverage for unsigned long long int and long long int
for Power 10 in int_128bit-runnable.c.
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.
vcmpnet): Move definitions to Altivec stanza.
* config/rs6000/altivec.md (vcmpneb, vcmpneh, vcmpnew): New
define_expand.
---
gcc/config/rs6000/altivec.md | 12
gcc/config/rs6000/rs6000-builtins.def | 18 +-
2 files changed, 21 insertions(+), 9 deletions(-)
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 "register_operand" "v")
diff --git a/gcc/config/rs6000/rs6000-builtins.def
b/gcc/config/rs6000/rs6000-builtins.def
index 638d0bc72ca..6b06fa8b34d 100644
--- a/gcc/config/rs6000/rs6000-builtins.def
+++ b/gcc/config/rs6000/rs6000-builtins.def
@@ -641,6 +641,15 @@
const int __builtin_altivec_vcmpgtuw_p (int, vsi, vsi);
VCMPGTUW_P vector_gtu_v4si_p {pred}
+ const vsc __builtin_altivec_vcmpneb (vsc, vsc);
+VCMPNEB altivec_vcmpne_v16qi {}
+
+ const vss __builtin_altivec_vcmpneh (vss, vss);
+VCMPNEH altivec_vcmpne_v8hi {}
+
+ const vsi __builtin_altivec_vcmpnew (vsi, vsi);
+VCMPNEW altivec_vcmpne_v4si {}
+
const vsi __builtin_altivec_vctsxs (vf, const int<5>);
VCTSXS altivec_vctsxs {}
@@ -2599,9 +2608,6 @@
const signed int __builtin_altivec_vcmpaew_p (vsi, vsi);
VCMPAEW_P vector_ae_v4si_p {}
- const vsc __builtin_altivec_vcmpneb (vsc, vsc);
-VCMPNEB vcmpneb {}
-
const signed int __builtin_altivec_vcmpneb_p (vsc, vsc);
VCMPNEB_P vector_ne_v16qi_p {}
@@ -2614,15 +2620,9 @@
const signed int __builtin_altivec_vcmpnefp_p (vf, vf);
VCMPNEFP_P vector_ne_v4sf_p {}
- const vss __builtin_altivec_vcmpneh (vss, vss);
-VCMPNEH vcmpneh {}
-
const signed int __builtin_altivec_vcmpneh_p (vss, vss);
VCMPNEH_P vector_ne_v8hi_p {}
- const vsi __builtin_altivec_vcmpnew (vsi, vsi);
-VCMPNEW vcmpnew {}
-
const signed int __builtin_altivec_vcmpnew_p (vsi, vsi);
VCMPNEW_P vector_ne_v4si_p {}
--
