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
