[PATCH, rs6000] rs6000: correct vector sign extend built-ins on Big Endian [PR108812]

[PATCH, rs6000] rs6000: correct vector sign extend built-ins on Big Endian [PR108812]

[HAO CHEN GUI]

Hi,
  This patch removes byte reverse operation before vector integer sign
extension on Big Endian. These built-ins require to sign extend the rightmost
element. So both BE and LE should do the same operation and the byte reversion
is no need. This patch fixes it. Now these built-ins have the same behavior on
all compilers. The test case is modified also.

  The patch passed regression test on Power Linux platforms.

Thanks
Gui Haochen

ChangeLog
rs6000: correct vector sign extend builtins on Big Endian

gcc/
	PR target/108812
	* config/rs6000/vsx.md (vsignextend_qi_<mode>): Remove byte reverse
	for Big Endian.
	(vsignextend_hi_<mode>): Likewise.
	(vsignextend_si_v2di): Remove.
	* config/rs6000/rs6000-builtins.def (__builtin_altivec_vsignextsw2d):
	Set bif-pattern to vsx_sign_extend_si_v2di.

gcc/testsuite/
	PR target/108812
	* gcc.target/powerpc/p9-sign_extend-runnable.c: Set different expected
	vectors for Big Endian.


patch.diff
diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
index f76f54793d7..059a455b388 100644
--- a/gcc/config/rs6000/rs6000-builtins.def
+++ b/gcc/config/rs6000/rs6000-builtins.def
@@ -2699,7 +2699,7 @@
     VSIGNEXTSH2W vsignextend_hi_v4si {}

   const vsll __builtin_altivec_vsignextsw2d (vsi);
-    VSIGNEXTSW2D vsignextend_si_v2di {}
+    VSIGNEXTSW2D vsx_sign_extend_si_v2di {}

   const vsc __builtin_altivec_vslv (vsc, vsc);
     VSLV vslv {}
diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
index 992fbc983be..9e9b33f56ab 100644
--- a/gcc/config/rs6000/vsx.md
+++ b/gcc/config/rs6000/vsx.md
@@ -4941,14 +4941,7 @@ (define_expand "vsignextend_qi_<mode>"
 	 UNSPEC_VSX_SIGN_EXTEND))]
   "TARGET_P9_VECTOR"
 {
-  if (BYTES_BIG_ENDIAN)
-    {
-      rtx tmp = gen_reg_rtx (V16QImode);
-      emit_insn (gen_altivec_vrevev16qi2(tmp, operands[1]));
-      emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], tmp));
-    }
-  else
-    emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], operands[1]));
+  emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], operands[1]));
   DONE;
 })

@@ -4968,14 +4961,7 @@ (define_expand "vsignextend_hi_<mode>"
 	 UNSPEC_VSX_SIGN_EXTEND))]
   "TARGET_P9_VECTOR"
 {
-  if (BYTES_BIG_ENDIAN)
-    {
-      rtx tmp = gen_reg_rtx (V8HImode);
-      emit_insn (gen_altivec_vrevev8hi2(tmp, operands[1]));
-      emit_insn (gen_vsx_sign_extend_hi_<mode>(operands[0], tmp));
-    }
-  else
-     emit_insn (gen_vsx_sign_extend_hi_<mode>(operands[0], operands[1]));
+  emit_insn (gen_vsx_sign_extend_hi_<mode>(operands[0], operands[1]));
   DONE;
 })

@@ -4987,24 +4973,6 @@ (define_insn "vsx_sign_extend_si_v2di"
   "vextsw2d %0,%1"
   [(set_attr "type" "vecexts")])

-(define_expand "vsignextend_si_v2di"
-  [(set (match_operand:V2DI 0 "vsx_register_operand" "=v")
-	(unspec:V2DI [(match_operand:V4SI 1 "vsx_register_operand" "v")]
-		     UNSPEC_VSX_SIGN_EXTEND))]
-  "TARGET_P9_VECTOR"
-{
-  if (BYTES_BIG_ENDIAN)
-    {
-       rtx tmp = gen_reg_rtx (V4SImode);
-
-       emit_insn (gen_altivec_vrevev4si2(tmp, operands[1]));
-       emit_insn (gen_vsx_sign_extend_si_v2di(operands[0], tmp));
-    }
-  else
-     emit_insn (gen_vsx_sign_extend_si_v2di(operands[0], operands[1]));
-  DONE;
-})
-
 ;; Sign extend DI to TI.  We provide both GPR targets and Altivec targets on
 ;; power10.  On earlier systems, the machine independent code will generate a
 ;; shift left to sign extend the 64-bit value to 128-bit.
diff --git a/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c b/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c
index fdcad019b96..03c0f1201e4 100644
--- a/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c
+++ b/gcc/testsuite/gcc.target/powerpc/p9-sign_extend-runnable.c
@@ -34,7 +34,12 @@ int main ()
   /* test sign extend byte to word */
   vec_arg_qi = (vector signed char) {1, 2, 3, 4, 5, 6, 7, 8,
 				     -1, -2, -3, -4, -5, -6, -7, -8};
+
+#ifdef __BIG_ENDIAN__
+  vec_expected_wi = (vector signed int) {4, 8, -4, -8};
+#else
   vec_expected_wi = (vector signed int) {1, 5, -1, -5};
+#endif

   vec_result_wi = vec_signexti (vec_arg_qi);

@@ -54,7 +59,12 @@ int main ()
   /* test sign extend byte to double */
   vec_arg_qi = (vector signed char){1, 2, 3, 4, 5, 6, 7, 8,
 				    -1, -2, -3, -4, -5, -6, -7, -8};
+
+#ifdef __BIG_ENDIAN__
+  vec_expected_di = (vector signed long long int){8, -8};
+#else
   vec_expected_di = (vector signed long long int){1, -1};
+#endif

   vec_result_di = vec_signextll(vec_arg_qi);

@@ -72,7 +82,12 @@ int main ()

   /* test sign extend short to word */
   vec_arg_hi = (vector signed short int){1, 2, 3, 4, -1, -2, -3, -4};
+
+#ifdef __BIG_ENDIAN__
+  vec_expected_wi = (vector signed int){2, 4, -2, -4};
+#else
   vec_expected_wi = (vector signed int){1, 3, -1, -3};
+#endif

   vec_result_wi = vec_signexti(vec_arg_hi);

@@ -90,7 +105,12 @@ int main ()

   /* test sign extend short to double word */
   vec_arg_hi = (vector signed short int ){1, 3, 5, 7,  -1, -3, -5, -7};
+
+#ifdef __BIG_ENDIAN__
+  vec_expected_di = (vector signed long long int){7, -7};
+#else
   vec_expected_di = (vector signed long long int){1, -1};
+#endif

   vec_result_di = vec_signextll(vec_arg_hi);

@@ -108,7 +128,12 @@ int main ()

   /* test sign extend word to double word */
   vec_arg_wi = (vector signed int ){1, 3, -1, -3};
+
+#ifdef __BIG_ENDIAN__
+  vec_expected_di = (vector signed long long int){3, -3};
+#else
   vec_expected_di = (vector signed long long int){1, -1};
+#endif

   vec_result_di = vec_signextll(vec_arg_wi);

Re: [PATCH, rs6000] rs6000: correct vector sign extend built-ins on Big Endian [PR108812]

[Kewen.Lin]

Hi Haochen,

Thanks for fixing this.

on 2023/3/27 14:16, HAO CHEN GUI wrote:
> Hi,
>   This patch removes byte reverse operation before vector integer sign
> extension on Big Endian. These built-ins require to sign extend the rightmost
> element. So both BE and LE should do the same operation and the byte reversion
> is no need. This patch fixes it. Now these built-ins have the same behavior on
> all compilers. The test case is modified also.

Nice, I think this change aligns with what's in the documentation:

"Each element of the result is produced by sign-extending the element of the input
vector that would fall in the least significant portion of the result element. For
example, a sign-extension of a vector signed char to a vector signed long long will
sign extend the rightmost byte of each doubleword."

> 
>   The patch passed regression test on Power Linux platforms.
> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> rs6000: correct vector sign extend builtins on Big Endian
> 
> gcc/
> 	PR target/108812
> 	* config/rs6000/vsx.md (vsignextend_qi_<mode>): Remove byte reverse
> 	for Big Endian.
> 	(vsignextend_hi_<mode>): Likewise.
> 	(vsignextend_si_v2di): Remove.
> 	* config/rs6000/rs6000-builtins.def (__builtin_altivec_vsignextsw2d):
> 	Set bif-pattern to vsx_sign_extend_si_v2di.
> 
> gcc/testsuite/
> 	PR target/108812
> 	* gcc.target/powerpc/p9-sign_extend-runnable.c: Set different expected
> 	vectors for Big Endian.
> 
> 
> patch.diff
> diff --git a/gcc/config/rs6000/rs6000-builtins.def b/gcc/config/rs6000/rs6000-builtins.def
> index f76f54793d7..059a455b388 100644
> --- a/gcc/config/rs6000/rs6000-builtins.def
> +++ b/gcc/config/rs6000/rs6000-builtins.def
> @@ -2699,7 +2699,7 @@
>      VSIGNEXTSH2W vsignextend_hi_v4si {}
> 
>    const vsll __builtin_altivec_vsignextsw2d (vsi);
> -    VSIGNEXTSW2D vsignextend_si_v2di {}
> +    VSIGNEXTSW2D vsx_sign_extend_si_v2di {}
> 
>    const vsc __builtin_altivec_vslv (vsc, vsc);
>      VSLV vslv {}
> diff --git a/gcc/config/rs6000/vsx.md b/gcc/config/rs6000/vsx.md
> index 992fbc983be..9e9b33f56ab 100644
> --- a/gcc/config/rs6000/vsx.md
> +++ b/gcc/config/rs6000/vsx.md
> @@ -4941,14 +4941,7 @@ (define_expand "vsignextend_qi_<mode>"
>  	 UNSPEC_VSX_SIGN_EXTEND))]
>    "TARGET_P9_VECTOR"
>  {
> -  if (BYTES_BIG_ENDIAN)
> -    {
> -      rtx tmp = gen_reg_rtx (V16QImode);
> -      emit_insn (gen_altivec_vrevev16qi2(tmp, operands[1]));
> -      emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], tmp));
> -    }
> -  else
> -    emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], operands[1]));
> +  emit_insn (gen_vsx_sign_extend_qi_<mode>(operands[0], operands[1]));
>    DONE;
>  })

I think the whole define_expand can be removed, we can just use the
define_insn names vsx_sign_extend_qi_* in rs6000-builtins.def (just
like what you changed for __builtin_altivec_vsignextsw2d).

This comment is also applied for vsx_sign_extend_hi_*,
vsx_sign_extend_si_* and vsx_sign_extend_v2di_*.

One interesting thing is that we used qi/hi/si in the name for
V16QI/V8HI/V4SI but used v2di for V2DI, could you also adjust the
names from vsx_sign_extend_{qi,hi,si}_* to ..._{v16qi,v8hi,v4si}_*
then make them adopt the same naming style?

BR,
Kewen

Re: [PATCH, rs6000] rs6000: correct vector sign extend built-ins on Big Endian [PR108812]

[Segher Boessenkool]

On Mon, Mar 27, 2023 at 03:14:26PM +0800, Kewen.Lin wrote:
> on 2023/3/27 14:16, HAO CHEN GUI wrote:
> >   This patch removes byte reverse operation before vector integer sign
> > extension on Big Endian. These built-ins require to sign extend the rightmost
> > element. So both BE and LE should do the same operation and the byte reversion
> > is no need. This patch fixes it. Now these built-ins have the same behavior on
> > all compilers. The test case is modified also.

When extending from sizes A to B the rightmost A in every B.  That is
the same in every endianness, yes -- it is what the machine insns do
after all, it has nothing to do with how the elements are numbered in
the ABI :-)

> I think the whole define_expand can be removed, we can just use the
> define_insn names vsx_sign_extend_qi_* in rs6000-builtins.def (just
> like what you changed for __builtin_altivec_vsignextsw2d).

A very welcome cleanup :-)

> One interesting thing is that we used qi/hi/si in the name for
> V16QI/V8HI/V4SI but used v2di for V2DI, could you also adjust the
> names from vsx_sign_extend_{qi,hi,si}_* to ..._{v16qi,v8hi,v4si}_*
> then make them adopt the same naming style?

Yes please :-)


Segher