mbox series

[00/12] tcg: Factor hrev{32,64}_{i32,i64,tl} out

Message ID 20230822124042.54739-1-philmd@linaro.org
Headers show
Series tcg: Factor hrev{32,64}_{i32,i64,tl} out | expand

Message

Philippe Mathieu-Daudé Aug. 22, 2023, 12:40 p.m. UTC
This series factor the "byteswap each halfword within a
32/64-bit value" code duplication as generic helpers.

Modulo the documentation added, there is a good negative
diff-stat, so I believe this is a win from a maintainance
point of view.

I used "hrev" to follow the other bswap/hswap/rev helpers
but it isn't very descriptive, so any better name suggestion
is welcomed.
(In particular because there are other patterns I'd like to
factor out and then naming is getting worse, such 'wrev').

Philippe Mathieu-Daudé (12):
  tcg/tcg-op: Factor tcg_gen_hrev32_i32() out
  target/arm: Use generic hrev32_i32() in ARM REV16 and VREV16 opcodes
  target/cris: Use generic hrev32_i32() in SWAPB opcode
  target/rx: Use generic hrev32_i32() in REVW opcode
  tcg/tcg-op: Factor tcg_gen_hrev64_i64() out
  target/mips: Use generic hrev64_i64() in DSBH opcode
  target/ppc: Use generic hrev64_i64() in BRH / BSWAP16x8 opcodes
  target/loongarch: Use generic hrev64_i64() in REVB.4H opcode
  tcg/tcg-op: Add tcg_gen_hrev32_i64() and tcg_gen_hrev_i64()
  target/arm: Use generic hrev_i64() in Aarch64 REV16 opcode
  target/loongarch: Use generic hrev64_i32() in REVB.2H opcode
  target/mips: Use generic hrev32_tl() in WSBH opcode

 docs/devel/tcg-ops.rst                      | 10 +++
 include/tcg/tcg-op-common.h                 |  4 +
 include/tcg/tcg-op.h                        |  2 +
 target/arm/tcg/translate-a32.h              |  1 -
 target/arm/tcg/translate-a64.c              | 11 +--
 target/arm/tcg/translate-neon.c             |  2 +-
 target/arm/tcg/translate.c                  | 14 +---
 target/cris/translate.c                     | 20 +----
 target/mips/tcg/translate.c                 | 24 +-----
 target/ppc/translate.c                      | 10 +--
 target/rx/translate.c                       |  8 +-
 tcg/tcg-op.c                                | 81 ++++++++++++++++++---
 target/cris/translate_v10.c.inc             |  2 +-
 target/loongarch/insn_trans/trans_bit.c.inc | 30 +-------
 target/ppc/translate/vsx-impl.c.inc         | 19 +----
 15 files changed, 99 insertions(+), 139 deletions(-)

Comments

Richard Henderson Aug. 22, 2023, 3:37 p.m. UTC | #1
On 8/22/23 05:40, Philippe Mathieu-Daudé wrote:
> This series factor the "byteswap each halfword within a
> 32/64-bit value" code duplication as generic helpers.
> 
> Modulo the documentation added, there is a good negative
> diff-stat, so I believe this is a win from a maintainance
> point of view.
> 
> I used "hrev" to follow the other bswap/hswap/rev helpers
> but it isn't very descriptive, so any better name suggestion
> is welcomed.
> (In particular because there are other patterns I'd like to
> factor out and then naming is getting worse, such 'wrev').'

I applaud the code factor, but the names are poor.

The "h" does not match the size of the elements being swapped, which is "b".  The "32" 
is... what, the total count of bits modified?

Naming is hard, and I'm not sure what's best.

We have bswap32_i32, bswap32_i64, bswap64_i64.

Perhaps bswap16x2_i32, bswap16x4_i64, bswap16xN_tl, to indicate that we're bswaping 16-bit 
quantities, and "xN" to indicate that multiple 16-bit quantities are being swapped.

 From your subjects, it would appear we don't need bswap16x2_i64, with the upper 32-bits 
zero/signed/undefined.  But if we did, we should provide a flags argument of TCG_BSWAP_*.

That then extends to hswap32x2_i64 to swap halfwords within multiple words for mips DSHW 
et al.


r~
Nicholas Piggin Aug. 25, 2023, 7:58 a.m. UTC | #2
On Tue Aug 22, 2023 at 10:40 PM AEST, Philippe Mathieu-Daudé wrote:
> This series factor the "byteswap each halfword within a
> 32/64-bit value" code duplication as generic helpers.
>
> Modulo the documentation added, there is a good negative
> diff-stat, so I believe this is a win from a maintainance
> point of view.
>
> I used "hrev" to follow the other bswap/hswap/rev helpers
> but it isn't very descriptive, so any better name suggestion
> is welcomed.
> (In particular because there are other patterns I'd like to
> factor out and then naming is getting worse, such 'wrev').

I know bswap has alrady precedent, but you could drop the bit
size when it matches the operand size, hrev_i64.

Could possibly also follow ppc and call it brevh, which tells
you the units being swapped and the size they are swapped in.
Then you could add brevw or hrevw etc. and it might be a bit
less ambiguous.

Looks like a nice cleanup though.

Thanks,
Nick

>
> Philippe Mathieu-Daudé (12):
>   tcg/tcg-op: Factor tcg_gen_hrev32_i32() out
>   target/arm: Use generic hrev32_i32() in ARM REV16 and VREV16 opcodes
>   target/cris: Use generic hrev32_i32() in SWAPB opcode
>   target/rx: Use generic hrev32_i32() in REVW opcode
>   tcg/tcg-op: Factor tcg_gen_hrev64_i64() out
>   target/mips: Use generic hrev64_i64() in DSBH opcode
>   target/ppc: Use generic hrev64_i64() in BRH / BSWAP16x8 opcodes
>   target/loongarch: Use generic hrev64_i64() in REVB.4H opcode
>   tcg/tcg-op: Add tcg_gen_hrev32_i64() and tcg_gen_hrev_i64()
>   target/arm: Use generic hrev_i64() in Aarch64 REV16 opcode
>   target/loongarch: Use generic hrev64_i32() in REVB.2H opcode
>   target/mips: Use generic hrev32_tl() in WSBH opcode
>
>  docs/devel/tcg-ops.rst                      | 10 +++
>  include/tcg/tcg-op-common.h                 |  4 +
>  include/tcg/tcg-op.h                        |  2 +
>  target/arm/tcg/translate-a32.h              |  1 -
>  target/arm/tcg/translate-a64.c              | 11 +--
>  target/arm/tcg/translate-neon.c             |  2 +-
>  target/arm/tcg/translate.c                  | 14 +---
>  target/cris/translate.c                     | 20 +----
>  target/mips/tcg/translate.c                 | 24 +-----
>  target/ppc/translate.c                      | 10 +--
>  target/rx/translate.c                       |  8 +-
>  tcg/tcg-op.c                                | 81 ++++++++++++++++++---
>  target/cris/translate_v10.c.inc             |  2 +-
>  target/loongarch/insn_trans/trans_bit.c.inc | 30 +-------
>  target/ppc/translate/vsx-impl.c.inc         | 19 +----
>  15 files changed, 99 insertions(+), 139 deletions(-)