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

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

[Carl Love]

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_<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]));
+  });
+
 (define_insn "*altivec_vcmpgts<VI_char>_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 {}
 
-- 
2.37.2

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

[Kewen.Lin]

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

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

[Carl Love]

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