[PATCH] Add power10 BRD/BRW/BRH support.

[Segher Boessenkool]

Hi!

On Wed, Jul 01, 2020 at 01:04:02PM -0400, Michael Meissner wrote:
> This patch adds support for the Power10 BRD, BRW, and BRH instructions that do
> byte swapping on values in GPRs.

> I used '*' for the length attribute where it generates a single instruction.

More exactly: those that can use the default length calculation.

> I did not change the tests in the splitter as I don't see any easier way to do
> the test.  For reference, there are 3 cases:
> 
>    1) Power10 instruction swapping a GPR which does not get split
>    2) Non-power10 instruction splitting the insn into component parts; (and)
>    3) Power9 instruction swapping a vector register which does not get split.

>  (define_insn_and_split "bswaphi2_reg"

> +   brh %0,%1
>     #
>     xxbrh %x0,%x1"

> +   (set_attr "type" "shift,*,vecperm")
> +   (set_attr "isa" "p10,*,p9v")])

I don't think "shift" will be a good type for this, but we'll see.  (The
insn types are mostly for scheduling...  It is often hard to come up with
something good while there is only one implementation.  Power9 was quite
different from most older chips here.  Hopefully it can improve now we
also have Power10.

>  ;; 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"

(Not new in this patch of course...  There are no zero_extract insns
below, heh).

> @@ -2641,9 +2643,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" "p10,*,p9v")])

"length" should use "*" here, as well.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/bswap-brd.c
> @@ -0,0 +1,23 @@
> +/* { dg-do compile { target { powerpc*-*-* && lp64 } } } */

Everything in gcc.target/powerpc is powerpc*-*-* already.

Why does this neede lp64?  (Put that in a comment.)

> +/* This tests whether GCC generates the ISA 3.1 BRW byte swap instruction for
> +   GPR data, but generates XXBRW for data in a vector register.  */

This comment is wrong.  This tests for "brd" (and "xxbrd"), instead.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/bswap-brh.c
> @@ -0,0 +1,12 @@
> +/* { dg-do compile { target { powerpc*-*-* } } } */

powerpc*-*-* is default.  Don't use it here.

> +/* This tests whether GCC generates the ISA 3.1 16-bit byte swap
> +   instruction BRH.  */

"whether GCC generates "brh" to implement 16-bit byte swap" or such...
(the instruction swaps every pair of bytes).

Okay for trunk with those things fixed.  Thanks!


Segher