[1/3] PowerPC future: Add byte swap insns

Message ID 1591041713-24527-2-git-send-email-meissner@linux.ibm.com
State New
Headers show
Series
  • [1/3] PowerPC future: Add byte swap insns
Related show

Commit Message

Kewen.Lin via Gcc-patches June 1, 2020, 8:01 p.m.
Add support for generating BRH/BRW/BRD when -mcpu=future is used.

gcc/
2020-06-01  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/rs6000.md (bswaphi2_reg): If -mcpu=future,
	generate the BRH instruction.
	(bswapsi2_reg): If -mcpu=future, generate the BRW instruction.
	(bswapdi2): Rename bswapdi2_xxbrd to bswapdi2_hw.
	(bswapdi2_hw): Rename from bswapdi2_xxbrd.  If -mcpu=future,
	generate the BRD instruction.

testsuite/
2020-06-01  Michael Meissner  <meissner@linux.ibm.com>

	* gcc.target/powerpc/bswap64-5.c: New test.
---
 gcc/config/rs6000/rs6000.md                  | 44 +++++++++++++++-------------
 gcc/testsuite/gcc.target/powerpc/bswap64-5.c | 42 ++++++++++++++++++++++++++
 2 files changed, 66 insertions(+), 20 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/bswap64-5.c

-- 
1.8.3.1

Comments

Kewen.Lin via Gcc-patches June 1, 2020, 11:04 p.m. | #1
On Mon, 2020-06-01 at 16:01 -0400, Michael Meissner via Gcc-patches
wrote:
> Add support for generating BRH/BRW/BRD when -mcpu=future is used.

> 


Hi,


> gcc/

> 2020-06-01  Michael Meissner  <meissner@linux.ibm.com>

> 

> 	* config/rs6000/rs6000.md (bswaphi2_reg): If -mcpu=future,

> 	generate the BRH instruction.

> 	(bswapsi2_reg): If -mcpu=future, generate the BRW instruction.

> 	(bswapdi2): Rename bswapdi2_xxbrd to bswapdi2_hw.

> 	(bswapdi2_hw): Rename from bswapdi2_xxbrd.  If -mcpu=future,

> 	generate the BRD instruction.


The "If -mcpu=future" blurbs there could probably be dropped.

> 

> testsuite/

> 2020-06-01  Michael Meissner  <meissner@linux.ibm.com>

> 

> 	* gcc.target/powerpc/bswap64-5.c: New test.

> ---

>  gcc/config/rs6000/rs6000.md                  | 44 +++++++++++++++-------------

>  gcc/testsuite/gcc.target/powerpc/bswap64-5.c | 42 ++++++++++++++++++++++++++

>  2 files changed, 66 insertions(+), 20 deletions(-)

>  create mode 100644 gcc/testsuite/gcc.target/powerpc/bswap64-5.c

> 


<snip>

> diff --git a/gcc/testsuite/gcc.target/powerpc/bswap64-5.c

> b/gcc/testsuite/gcc.target/powerpc/bswap64-5.c

> new file mode 100644

> index 0000000..9183e16

> --- /dev/null

> +++ b/gcc/testsuite/gcc.target/powerpc/bswap64-5.c

> @@ -0,0 +1,42 @@

> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */

> +/* { dg-require-effective-target powerpc_future_ok } */

> +/* { dg-options "-O2 -mdejagnu-cpu=future" } */

> +

> +/* This tests whether -mcpu=future generates the new byte swap

> +   instructions (brd, brw, brh).  */


s/new//
(It's only new until it's not).

Aside from those nits, lgtm.
thanks
-Will
Segher Boessenkool June 9, 2020, 8:38 p.m. | #2
Hi!

On Mon, Jun 01, 2020 at 04:01:51PM -0400, Michael Meissner wrote:
> Add support for generating BRH/BRW/BRD when -mcpu=future is used.


> 	* config/rs6000/rs6000.md (bswaphi2_reg): If -mcpu=future,

> 	generate the BRH instruction.


"Add alternative for brh."?  If the changelog ruthlessly says what the
patch does, not how or why or anything, this is much more useful
information (anything less compact can be found elsewhere easily).  It
helps reviewing, and also helps people who have to do archaeology.

If people did great commit messages we would not need changelogs.  As
long as that is not happening, we need changelogs.

> 	(bswapsi2_reg): If -mcpu=future, generate the BRW instruction.

> 	(bswapdi2): Rename bswapdi2_xxbrd to bswapdi2_hw.

> 	(bswapdi2_hw): Rename from bswapdi2_xxbrd.  If -mcpu=future,

> 	generate the BRD instruction.


"hw" is meaningless.  Call it bswapdi2_brd, perhaps?  That can easily
stand for "brd or xxbrd" :-)

>  (define_insn_and_split "bswaphi2_reg"

> -  [(set (match_operand:HI 0 "gpc_reg_operand" "=&r,wa")

> +  [(set (match_operand:HI 0 "gpc_reg_operand" "=r,&r,wa")

>  	(bswap:HI

> -	 (match_operand:HI 1 "gpc_reg_operand" "r,wa")))

> -   (clobber (match_scratch:SI 2 "=&r,X"))]

> +	 (match_operand:HI 1 "gpc_reg_operand" "r,r,wa")))

> +   (clobber (match_scratch:SI 2 "=X,&r,X"))]

>    ""

>    "@

> +   brh %0,%1

>     #

>     xxbrh %x0,%x1"

> -  "reload_completed && int_reg_operand (operands[0], HImode)"

> +  "reload_completed && !TARGET_FUTURE && int_reg_operand (operands[0], HImode)"


This condition is mightily complex, mixing a few concepts.  It probably
works fine, but this isn't so obvious.  Can it be improved some way?

> @@ -2609,21 +2610,22 @@ (define_insn_and_split "bswaphi2_reg"

>    operands[3] = simplify_gen_subreg (SImode, operands[0], HImode, 0);

>    operands[4] = simplify_gen_subreg (SImode, operands[1], HImode, 0);

>  }

> -  [(set_attr "length" "12,4")

> -   (set_attr "type" "*,vecperm")

> -   (set_attr "isa" "*,p9v")])

> +  [(set_attr "length" "4,12,4")


"*,12,*" perhaps?

> +   (set_attr "type" "shift,*,vecperm")


I don't know if "shift" is a good type at all, but it'll be adjusted for
the scheduling models anyway.  Okay.

> --- /dev/null

> +++ b/gcc/testsuite/gcc.target/powerpc/bswap64-5.c

> @@ -0,0 +1,42 @@

> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */


Why does this test only on 64-bit systems?  Why is the file called
"bswap64", while it tests smaller sizes as well?

Split the test into two?

Rest looks fine.


Segher

Patch

diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 0aa5265..3310b4b 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -2585,15 +2585,16 @@  (define_insn "bswap<mode>2_store"
   [(set_attr "type" "store")])
 
 (define_insn_and_split "bswaphi2_reg"
-  [(set (match_operand:HI 0 "gpc_reg_operand" "=&r,wa")
+  [(set (match_operand:HI 0 "gpc_reg_operand" "=r,&r,wa")
 	(bswap:HI
-	 (match_operand:HI 1 "gpc_reg_operand" "r,wa")))
-   (clobber (match_scratch:SI 2 "=&r,X"))]
+	 (match_operand:HI 1 "gpc_reg_operand" "r,r,wa")))
+   (clobber (match_scratch:SI 2 "=X,&r,X"))]
   ""
   "@
+   brh %0,%1
    #
    xxbrh %x0,%x1"
-  "reload_completed && int_reg_operand (operands[0], HImode)"
+  "reload_completed && !TARGET_FUTURE && int_reg_operand (operands[0], HImode)"
   [(set (match_dup 3)
 	(and:SI (lshiftrt:SI (match_dup 4)
 			     (const_int 8))
@@ -2609,21 +2610,22 @@  (define_insn_and_split "bswaphi2_reg"
   operands[3] = simplify_gen_subreg (SImode, operands[0], HImode, 0);
   operands[4] = simplify_gen_subreg (SImode, operands[1], HImode, 0);
 }
-  [(set_attr "length" "12,4")
-   (set_attr "type" "*,vecperm")
-   (set_attr "isa" "*,p9v")])
+  [(set_attr "length" "4,12,4")
+   (set_attr "type" "shift,*,vecperm")
+   (set_attr "isa" "fut,*,p9v")])
 
 ;; We are always BITS_BIG_ENDIAN, so the bit positions below in
 ;; zero_extract insns do not change for -mlittle.
 (define_insn_and_split "bswapsi2_reg"
-  [(set (match_operand:SI 0 "gpc_reg_operand" "=&r,wa")
+  [(set (match_operand:SI 0 "gpc_reg_operand" "=r,&r,wa")
 	(bswap:SI
-	 (match_operand:SI 1 "gpc_reg_operand" "r,wa")))]
+	 (match_operand:SI 1 "gpc_reg_operand" "r,r,wa")))]
   ""
   "@
+   brw %0,%1
    #
    xxbrw %x0,%x1"
-  "reload_completed && int_reg_operand (operands[0], SImode)"
+  "reload_completed && !TARGET_FUTURE && int_reg_operand (operands[0], SImode)"
   [(set (match_dup 0)					; DABC
 	(rotate:SI (match_dup 1)
 		   (const_int 24)))
@@ -2640,9 +2642,9 @@  (define_insn_and_split "bswapsi2_reg"
 		(and:SI (match_dup 0)
 			(const_int -256))))]
   ""
-  [(set_attr "length" "12,4")
-   (set_attr "type" "*,vecperm")
-   (set_attr "isa" "*,p9v")])
+  [(set_attr "length" "4,12,4")
+   (set_attr "type" "shift,*,vecperm")
+   (set_attr "isa" "fut,*,p9v")])
 
 ;; On systems with LDBRX/STDBRX generate the loads/stores directly, just like
 ;; we do for L{H,W}BRX and ST{H,W}BRX above.  If not, we have to generate more
@@ -2675,7 +2677,7 @@  (define_expand "bswapdi2"
 	  emit_insn (gen_bswapdi2_store (dest, src));
         }
       else if (TARGET_P9_VECTOR)
-	emit_insn (gen_bswapdi2_xxbrd (dest, src));
+	emit_insn (gen_bswapdi2_hw (dest, src));
       else
 	emit_insn (gen_bswapdi2_reg (dest, src));
       DONE;
@@ -2706,13 +2708,15 @@  (define_insn "bswapdi2_store"
   "stdbrx %1,%y0"
   [(set_attr "type" "store")])
 
-(define_insn "bswapdi2_xxbrd"
-  [(set (match_operand:DI 0 "gpc_reg_operand" "=wa")
-	(bswap:DI (match_operand:DI 1 "gpc_reg_operand" "wa")))]
+(define_insn "bswapdi2_hw"
+  [(set (match_operand:DI 0 "gpc_reg_operand" "=r,wa")
+	(bswap:DI (match_operand:DI 1 "gpc_reg_operand" "r,wa")))]
   "TARGET_P9_VECTOR"
-  "xxbrd %x0,%x1"
-  [(set_attr "type" "vecperm")
-   (set_attr "isa" "p9v")])
+  "@
+   brd %0,%1
+   xxbrd %x0,%x1"
+  [(set_attr "type" "shift,vecperm")
+   (set_attr "isa" "fut,p9v")])
 
 (define_insn "bswapdi2_reg"
   [(set (match_operand:DI 0 "gpc_reg_operand" "=&r")
diff --git a/gcc/testsuite/gcc.target/powerpc/bswap64-5.c b/gcc/testsuite/gcc.target/powerpc/bswap64-5.c
new file mode 100644
index 0000000..9183e16
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/bswap64-5.c
@@ -0,0 +1,42 @@ 
+/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */
+/* { dg-require-effective-target powerpc_future_ok } */
+/* { dg-options "-O2 -mdejagnu-cpu=future" } */
+
+/* This tests whether -mcpu=future generates the new byte swap
+   instructions (brd, brw, brh).  */
+
+unsigned short
+bswap_short (unsigned short a)
+{
+  return __builtin_bswap16 (a); /* { dg-final { scan-assembler {\mbrh\M} } } */
+}
+
+unsigned int
+bswap_int (unsigned int a)
+{
+  return __builtin_bswap32 (a); /* { dg-final { scan-assembler {\mbrw\M} } } */
+}
+
+unsigned long
+bswap_long (unsigned long a)
+{
+  return __builtin_bswap64 (a); /* { dg-final { scan-assembler {\mbrd\M} } } */
+}
+
+double
+bswap_int_dbl (unsigned int a)
+{
+  unsigned int b = a;
+  __asm__ (" # %x0" : "+wa" (b));
+  /* { dg-final { scan-assembler {\mxxbrw\M} } } */
+  return (double) __builtin_bswap32 (b);
+}
+
+double
+bswap_long_dbl (unsigned long a)
+{
+  unsigned long b = a;
+  __asm__ (" # %x0" : "+wa" (b));
+  /* { dg-final { scan-assembler {\mxxbrd\M} } } */
+  return (double) __builtin_bswap64 (b);
+}