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

Message ID adf6f3a2-0b43-4155-b564-729cc0a9be9d@linux.ibm.com
State New
Headers
Series [rs6000] Use bcdsub. instead of bcdadd. for bcd invalid number checking |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_gcc_check--master-arm success Testing passed

Commit Message

HAO CHEN GUI April 18, 2024, 2:01 a.m. UTC
  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
  

Comments

Kewen.Lin April 18, 2024, 3:14 a.m. UTC | #1
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 } } */
>
  
Segher Boessenkool April 18, 2024, 10:35 a.m. UTC | #2
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
  

Patch

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 } } */