[PATCH, rs6000] Use bcdsub. instead of bcdadd. for bcd invalid number checking

[PATCH, rs6000] Use bcdsub. instead of bcdadd. for bcd invalid number checking

[HAO CHEN GUI]

Hi,
  This patch replace bcdadd. with bcdsub. for bcd invalid number checking.
bcdadd on two same numbers might cause overflow which also set
overflow/invalid bit so that we can't distinguish it's invalid or overflow.
The bcdsub doesn't have the problem as subtracting on two same number never
causes overflow.

  Bootstrapped and tested on powerpc64-linux BE and LE with no
regressions. Is it OK for the trunk?

Thanks
Gui Haochen

ChangeLog
rs6000: Use bcdsub. instead of bcdadd. for bcd invalid number checking

bcdadd. might causes overflow which also set the overflow/invalid bit.
bcdsub. doesn't have the issue when do subtracting on two same bcd number.

gcc/
	* config/rs6000/altivec.md (*bcdinvalid_<mode>): Replace bcdadd
	with bcdsub.
	(bcdinvalid_<mode>): Likewise.

gcc/testsuite/
	* gcc.target/powerpc/bcd-4.c: Adjust the number of bcdadd and
	bcdsub.

patch.diff
diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
index 4d4c94ff0a0..bb20441c096 100644
--- a/gcc/config/rs6000/altivec.md
+++ b/gcc/config/rs6000/altivec.md
@@ -4586,18 +4586,18 @@ (define_insn "*bcdinvalid_<mode>"
   [(set (reg:CCFP CR6_REGNO)
 	(compare:CCFP
 	 (unspec:V2DF [(match_operand:VBCD 1 "register_operand" "v")]
-		      UNSPEC_BCDADD)
+		      UNSPEC_BCDSUB)
 	 (match_operand:V2DF 2 "zero_constant" "j")))
    (clobber (match_scratch:VBCD 0 "=v"))]
   "TARGET_P8_VECTOR"
-  "bcdadd. %0,%1,%1,0"
+  "bcdsub. %0,%1,%1,0"
   [(set_attr "type" "vecsimple")])

 (define_expand "bcdinvalid_<mode>"
   [(parallel [(set (reg:CCFP CR6_REGNO)
 		   (compare:CCFP
 		    (unspec:V2DF [(match_operand:VBCD 1 "register_operand")]
-				 UNSPEC_BCDADD)
+				 UNSPEC_BCDSUB)
 		    (match_dup 2)))
 	      (clobber (match_scratch:VBCD 3))])
    (set (match_operand:SI 0 "register_operand")
diff --git a/gcc/testsuite/gcc.target/powerpc/bcd-4.c b/gcc/testsuite/gcc.target/powerpc/bcd-4.c
index 2c7041c4d32..6d2c59ef792 100644
--- a/gcc/testsuite/gcc.target/powerpc/bcd-4.c
+++ b/gcc/testsuite/gcc.target/powerpc/bcd-4.c
@@ -2,8 +2,8 @@
 /* { dg-require-effective-target int128 } */
 /* { dg-require-effective-target p9vector_hw } */
 /* { dg-options "-mdejagnu-cpu=power9 -O2 -save-temps" } */
-/* { dg-final { scan-assembler-times {\mbcdadd\M} 7 } } */
-/* { dg-final { scan-assembler-times {\mbcdsub\M} 18 } } */
+/* { dg-final { scan-assembler-times {\mbcdadd\M} 5 } } */
+/* { dg-final { scan-assembler-times {\mbcdsub\M} 20 } } */
 /* { dg-final { scan-assembler-times {\mbcds\M} 2 } } */
 /* { dg-final { scan-assembler-times {\mdenbcdq\M} 1 } } */

Re: [PATCH, rs6000] Use bcdsub. instead of bcdadd. for bcd invalid number checking

[Kewen.Lin]

Hi,

on 2024/4/18 10:01, HAO CHEN GUI wrote:
> Hi,
>   This patch replace bcdadd. with bcdsub. for bcd invalid number checking.
> bcdadd on two same numbers might cause overflow which also set
> overflow/invalid bit so that we can't distinguish it's invalid or overflow.
> The bcdsub doesn't have the problem as subtracting on two same number never
> causes overflow.
> 
>   Bootstrapped and tested on powerpc64-linux BE and LE with no
> regressions. Is it OK for the trunk?

Considering that this issue affects some basic functionality of bcd bifs
and the fix itself is simple and very safe, OK for trunk, thanks for fixing!

BR,
Kewen

> 
> Thanks
> Gui Haochen
> 
> ChangeLog
> rs6000: Use bcdsub. instead of bcdadd. for bcd invalid number checking
> 
> bcdadd. might causes overflow which also set the overflow/invalid bit.
> bcdsub. doesn't have the issue when do subtracting on two same bcd number.
> 
> gcc/
> 	* config/rs6000/altivec.md (*bcdinvalid_<mode>): Replace bcdadd
> 	with bcdsub.
> 	(bcdinvalid_<mode>): Likewise.
> 
> gcc/testsuite/
> 	* gcc.target/powerpc/bcd-4.c: Adjust the number of bcdadd and
> 	bcdsub.
> 
> patch.diff
> diff --git a/gcc/config/rs6000/altivec.md b/gcc/config/rs6000/altivec.md
> index 4d4c94ff0a0..bb20441c096 100644
> --- a/gcc/config/rs6000/altivec.md
> +++ b/gcc/config/rs6000/altivec.md
> @@ -4586,18 +4586,18 @@ (define_insn "*bcdinvalid_<mode>"
>    [(set (reg:CCFP CR6_REGNO)
>  	(compare:CCFP
>  	 (unspec:V2DF [(match_operand:VBCD 1 "register_operand" "v")]
> -		      UNSPEC_BCDADD)
> +		      UNSPEC_BCDSUB)
>  	 (match_operand:V2DF 2 "zero_constant" "j")))
>     (clobber (match_scratch:VBCD 0 "=v"))]
>    "TARGET_P8_VECTOR"
> -  "bcdadd. %0,%1,%1,0"
> +  "bcdsub. %0,%1,%1,0"
>    [(set_attr "type" "vecsimple")])
> 
>  (define_expand "bcdinvalid_<mode>"
>    [(parallel [(set (reg:CCFP CR6_REGNO)
>  		   (compare:CCFP
>  		    (unspec:V2DF [(match_operand:VBCD 1 "register_operand")]
> -				 UNSPEC_BCDADD)
> +				 UNSPEC_BCDSUB)
>  		    (match_dup 2)))
>  	      (clobber (match_scratch:VBCD 3))])
>     (set (match_operand:SI 0 "register_operand")
> diff --git a/gcc/testsuite/gcc.target/powerpc/bcd-4.c b/gcc/testsuite/gcc.target/powerpc/bcd-4.c
> index 2c7041c4d32..6d2c59ef792 100644
> --- a/gcc/testsuite/gcc.target/powerpc/bcd-4.c
> +++ b/gcc/testsuite/gcc.target/powerpc/bcd-4.c
> @@ -2,8 +2,8 @@
>  /* { dg-require-effective-target int128 } */
>  /* { dg-require-effective-target p9vector_hw } */
>  /* { dg-options "-mdejagnu-cpu=power9 -O2 -save-temps" } */
> -/* { dg-final { scan-assembler-times {\mbcdadd\M} 7 } } */
> -/* { dg-final { scan-assembler-times {\mbcdsub\M} 18 } } */
> +/* { dg-final { scan-assembler-times {\mbcdadd\M} 5 } } */
> +/* { dg-final { scan-assembler-times {\mbcdsub\M} 20 } } */
>  /* { dg-final { scan-assembler-times {\mbcds\M} 2 } } */
>  /* { dg-final { scan-assembler-times {\mdenbcdq\M} 1 } } */
> 

Re: [PATCH, rs6000] Use bcdsub. instead of bcdadd. for bcd invalid number checking

[Segher Boessenkool]

On Thu, Apr 18, 2024 at 11:14:42AM +0800, Kewen.Lin wrote:
> on 2024/4/18 10:01, HAO CHEN GUI wrote:
> >   This patch replace bcdadd. with bcdsub. for bcd invalid number checking.
> > bcdadd on two same numbers might cause overflow which also set
> > overflow/invalid bit so that we can't distinguish it's invalid or overflow.
> > The bcdsub doesn't have the problem as subtracting on two same number never
> > causes overflow.
> > 
> >   Bootstrapped and tested on powerpc64-linux BE and LE with no
> > regressions. Is it OK for the trunk?
> 
> Considering that this issue affects some basic functionality of bcd bifs
> and the fix itself is simple and very safe, OK for trunk, thanks for fixing!

Yup.  If a number X is invalid the X-X calculation might set the
overflow flag as well, but we cannot see that difference at all anyway,
it always has set the invalid flag after all.

Thanks!


Segher