Re: [Qemu-devel] [PATCH 27/26] tcg-aarch64: Introduce tcg_out_insn_3312, _3310, _3313

[Claudio Fontana <claudio.fontana@huawei.com>]

On 07.04.2014 20:34, Richard Henderson wrote:
> Merge TCGMemOp size, AArch64LdstType type and a few stray opcode bits
> into a single I3312_* argument, eliminating some magic numbers from
> helper functions.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/aarch64/tcg-target.c | 129 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 76 insertions(+), 53 deletions(-)
> ---
> 
> I'm not really sure how much clearer this is, especially since we do
> have to re-extract the size within tcg_out_ldst.  But it does at least
> eliminate some of the magic numbers within the helpers.
> 
> Thoughts?
> 
> 
> r~
> 
> 
> 
> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c
> index ab4cd25..324a452 100644
> --- a/tcg/aarch64/tcg-target.c
> +++ b/tcg/aarch64/tcg-target.c
> @@ -271,6 +271,28 @@ typedef enum {
>      I3207_BLR       = 0xd63f0000,
>      I3207_RET       = 0xd65f0000,
>  
> +    /* Load/store register.  Described here as 3.3.12, but the helper
> +       that emits them can transform to 3.3.10 or 3.3.13.  */
> +    I3312_STRB      = 0x38000000 | LDST_ST << 22 | MO_8 << 30,
> +    I3312_STRH      = 0x38000000 | LDST_ST << 22 | MO_16 << 30,
> +    I3312_STRW      = 0x38000000 | LDST_ST << 22 | MO_32 << 30,
> +    I3312_STRX      = 0x38000000 | LDST_ST << 22 | MO_64 << 30,
> +
> +    I3312_LDRB      = 0x38000000 | LDST_LD << 22 | MO_8 << 30,
> +    I3312_LDRH      = 0x38000000 | LDST_LD << 22 | MO_16 << 30,
> +    I3312_LDRW      = 0x38000000 | LDST_LD << 22 | MO_32 << 30,
> +    I3312_LDRX      = 0x38000000 | LDST_LD << 22 | MO_64 << 30,
> +
> +    I3312_LDRSBW    = 0x38000000 | LDST_LD_S_W << 22 | MO_8 << 30,
> +    I3312_LDRSHW    = 0x38000000 | LDST_LD_S_W << 22 | MO_16 << 30,
> +
> +    I3312_LDRSBX    = 0x38000000 | LDST_LD_S_X << 22 | MO_8 << 30,
> +    I3312_LDRSHX    = 0x38000000 | LDST_LD_S_X << 22 | MO_16 << 30,
> +    I3312_LDRSWX    = 0x38000000 | LDST_LD_S_X << 22 | MO_32 << 30,
> +
> +    I3312_TO_I3310  = 0x00206800,
> +    I3312_TO_I3313  = 0x01000000,
> +
>      /* Load/store register pair instructions.  */
>      I3314_LDP       = 0x28400000,
>      I3314_STP       = 0x28000000,
> @@ -482,21 +504,25 @@ static void tcg_out_insn_3509(TCGContext *s, AArch64Insn insn, TCGType ext,
>      tcg_out32(s, insn | ext << 31 | rm << 16 | ra << 10 | rn << 5 | rd);
>  }
>  
> +static void tcg_out_insn_3310(TCGContext *s, AArch64Insn insn,
> +                              TCGReg rd, TCGReg base, TCGReg regoff)
> +{
> +    /* Note the AArch64Insn constants above are for C3.3.12.  Adjust.  */
> +    tcg_out32(s, insn | I3312_TO_I3310 | regoff << 16 | base << 5 | rd);
> +}
> +
>  
> -static void tcg_out_ldst_9(TCGContext *s, TCGMemOp size, AArch64LdstType type,
> -                           TCGReg rd, TCGReg rn, intptr_t offset)
> +static void tcg_out_insn_3312(TCGContext *s, AArch64Insn insn,
> +                              TCGReg rd, TCGReg rn, intptr_t offset)
>  {
> -    /* use LDUR with BASE register with 9bit signed unscaled offset */
> -    tcg_out32(s, 0x38000000 | size << 30 | type << 22
> -              | (offset & 0x1ff) << 12 | rn << 5 | rd);
> +    tcg_out32(s, insn | (offset & 0x1ff) << 12 | rn << 5 | rd);
>  }
>  
> -/* tcg_out_ldst_12 expects a scaled unsigned immediate offset */
> -static void tcg_out_ldst_12(TCGContext *s, TCGMemOp size, AArch64LdstType type,
> -                            TCGReg rd, TCGReg rn, tcg_target_ulong scaled_uimm)
> +static void tcg_out_insn_3313(TCGContext *s, AArch64Insn insn,
> +                              TCGReg rd, TCGReg rn, uintptr_t scaled_uimm)
>  {
> -    tcg_out32(s, 0x39000000 | size << 30 | type << 22
> -              | scaled_uimm << 10 | rn << 5 | rd);
> +    /* Note the AArch64Insn constants above are for C3.3.12.  Adjust.  */
> +    tcg_out32(s, insn | I3312_TO_I3313 | scaled_uimm << 10 | rn << 5 | rd);
>  }
>  
>  /* Register to register move using ORR (shifted register with no shift). */
> @@ -634,35 +660,32 @@ static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg rd,
>      }
>  }
>  
> -static void tcg_out_ldst_r(TCGContext *s, TCGMemOp size, AArch64LdstType type,
> -                           TCGReg rd, TCGReg base, TCGReg regoff)
> -{
> -    tcg_out32(s, 0x38206800 | size << 30 | type << 22
> -              | regoff << 16 | base << 5 | rd);
> -}
> +/* Define something more legible for general use.  */
> +#define tcg_out_ldst_r  tcg_out_insn_3310
>  
> -/* solve the whole ldst problem */
> -static void tcg_out_ldst(TCGContext *s, TCGMemOp size, AArch64LdstType type,
> +static void tcg_out_ldst(TCGContext *s, AArch64Insn insn,
>                           TCGReg rd, TCGReg rn, intptr_t offset)
>  {
> +    TCGMemOp size = (uint32_t)insn >> 30;
> +
>      /* If the offset is naturally aligned and in range, then we can
>         use the scaled uimm12 encoding */
>      if (offset >= 0 && !(offset & ((1 << size) - 1))) {
> -        tcg_target_ulong scaled_uimm = offset >> size;
> +        uintptr_t scaled_uimm = offset >> size;
>          if (scaled_uimm <= 0xfff) {
> -            tcg_out_ldst_12(s, size, type, rd, rn, scaled_uimm);
> +            tcg_out_insn_3313(s, insn, rd, rn, scaled_uimm);
>              return;
>          }
>      }
>  
>      if (offset >= -256 && offset < 256) {
> -        tcg_out_ldst_9(s, size, type, rd, rn, offset);
> +        tcg_out_insn_3312(s, insn, rd, rn, offset);
>          return;
>      }
>  
>      /* Worst-case scenario, move offset to temp register, use reg offset.  */
>      tcg_out_movi(s, TCG_TYPE_I64, TCG_REG_TMP, offset);
> -    tcg_out_ldst_r(s, size, type, rd, rn, TCG_REG_TMP);
> +    tcg_out_ldst_r(s, insn, rd, rn, TCG_REG_TMP);
>  }
>  
>  static inline void tcg_out_mov(TCGContext *s,
> @@ -676,14 +699,14 @@ static inline void tcg_out_mov(TCGContext *s,
>  static inline void tcg_out_ld(TCGContext *s, TCGType type, TCGReg arg,
>                                TCGReg arg1, intptr_t arg2)
>  {
> -    tcg_out_ldst(s, type == TCG_TYPE_I64 ? MO_64 : MO_32, LDST_LD,
> +    tcg_out_ldst(s, type == TCG_TYPE_I32 ? I3312_LDRW : I3312_LDRX,
>                   arg, arg1, arg2);
>  }
>  
>  static inline void tcg_out_st(TCGContext *s, TCGType type, TCGReg arg,
>                                TCGReg arg1, intptr_t arg2)
>  {
> -    tcg_out_ldst(s, type == TCG_TYPE_I64 ? MO_64 : MO_32, LDST_ST,
> +    tcg_out_ldst(s, type == TCG_TYPE_I32 ? I3312_STRW : I3312_STRX,
>                   arg, arg1, arg2);
>  }
>  
> @@ -1081,12 +1104,12 @@ static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, TCGMemOp s_bits,
>  
>      /* Merge "low bits" from tlb offset, load the tlb comparator into X0.
>         X0 = load [X2 + (tlb_offset & 0x000fff)] */
> -    tcg_out_ldst(s, TARGET_LONG_BITS == 64 ? MO_64 : MO_32,
> -                 LDST_LD, TCG_REG_X0, TCG_REG_X2, tlb_offset & 0xfff);
> +    tcg_out_ldst(s, TARGET_LONG_BITS == 32 ? I3312_LDRW : I3312_LDRX,
> +                 TCG_REG_X0, TCG_REG_X2, tlb_offset & 0xfff);
>  
>      /* Load the tlb addend. Do that early to avoid stalling.
>         X1 = load [X2 + (tlb_offset & 0xfff) + offsetof(addend)] */
> -    tcg_out_ldst(s, MO_64, LDST_LD, TCG_REG_X1, TCG_REG_X2,
> +    tcg_out_ldst(s, I3312_LDRX, TCG_REG_X1, TCG_REG_X2,
>                   (tlb_offset & 0xfff) + (offsetof(CPUTLBEntry, addend)) -
>                   (is_read ? offsetof(CPUTLBEntry, addr_read)
>                    : offsetof(CPUTLBEntry, addr_write)));
> @@ -1108,43 +1131,43 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop,
>  
>      switch (memop & MO_SSIZE) {
>      case MO_UB:
> -        tcg_out_ldst_r(s, MO_8, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, off_r);
>          break;
>      case MO_SB:
> -        tcg_out_ldst_r(s, MO_8, LDST_LD_S_X, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_LDRSBX, data_r, addr_r, off_r);
>          break;
>      case MO_UW:
> -        tcg_out_ldst_r(s, MO_16, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_LDRH, data_r, addr_r, off_r);
>          if (bswap) {
>              tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r);
>          }
>          break;
>      case MO_SW:
>          if (bswap) {
> -            tcg_out_ldst_r(s, MO_16, LDST_LD, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, I3312_LDRH, data_r, addr_r, off_r);
>              tcg_out_rev16(s, TCG_TYPE_I32, data_r, data_r);
>              tcg_out_sxt(s, TCG_TYPE_I64, MO_16, data_r, data_r);
>          } else {
> -            tcg_out_ldst_r(s, MO_16, LDST_LD_S_X, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, I3312_LDRSHX, data_r, addr_r, off_r);
>          }
>          break;
>      case MO_UL:
> -        tcg_out_ldst_r(s, MO_32, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_LDRW, data_r, addr_r, off_r);
>          if (bswap) {
>              tcg_out_rev(s, TCG_TYPE_I32, data_r, data_r);
>          }
>          break;
>      case MO_SL:
>          if (bswap) {
> -            tcg_out_ldst_r(s, MO_32, LDST_LD, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, I3312_LDRW, data_r, addr_r, off_r);
>              tcg_out_rev(s, TCG_TYPE_I32, data_r, data_r);
>              tcg_out_sxt(s, TCG_TYPE_I64, MO_32, data_r, data_r);
>          } else {
> -            tcg_out_ldst_r(s, MO_32, LDST_LD_S_X, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, I3312_LDRSWX, data_r, addr_r, off_r);
>          }
>          break;
>      case MO_Q:
> -        tcg_out_ldst_r(s, MO_64, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_LDRX, data_r, addr_r, off_r);
>          if (bswap) {
>              tcg_out_rev(s, TCG_TYPE_I64, data_r, data_r);
>          }
> @@ -1161,28 +1184,28 @@ static void tcg_out_qemu_st_direct(TCGContext *s, TCGMemOp memop,
>  
>      switch (memop & MO_SIZE) {
>      case MO_8:
> -        tcg_out_ldst_r(s, MO_8, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_STRB, data_r, addr_r, off_r);
>          break;
>      case MO_16:
>          if (bswap && data_r != TCG_REG_XZR) {
>              tcg_out_rev16(s, TCG_TYPE_I32, TCG_REG_TMP, data_r);
>              data_r = TCG_REG_TMP;
>          }
> -        tcg_out_ldst_r(s, MO_16, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_STRH, data_r, addr_r, off_r);
>          break;
>      case MO_32:
>          if (bswap && data_r != TCG_REG_XZR) {
>              tcg_out_rev(s, TCG_TYPE_I32, TCG_REG_TMP, data_r);
>              data_r = TCG_REG_TMP;
>          }
> -        tcg_out_ldst_r(s, MO_32, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_STRW, data_r, addr_r, off_r);
>          break;
>      case MO_64:
>          if (bswap && data_r != TCG_REG_XZR) {
>              tcg_out_rev(s, TCG_TYPE_I64, TCG_REG_TMP, data_r);
>              data_r = TCG_REG_TMP;
>          }
> -        tcg_out_ldst_r(s, MO_64, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, I3312_STRX, data_r, addr_r, off_r);
>          break;
>      default:
>          tcg_abort();
> @@ -1275,49 +1298,49 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>  
>      case INDEX_op_ld8u_i32:
>      case INDEX_op_ld8u_i64:
> -        tcg_out_ldst(s, MO_8, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRB, a0, a1, a2);
>          break;
>      case INDEX_op_ld8s_i32:
> -        tcg_out_ldst(s, MO_8, LDST_LD_S_W, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRSBW, a0, a1, a2);
>          break;
>      case INDEX_op_ld8s_i64:
> -        tcg_out_ldst(s, MO_8, LDST_LD_S_X, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRSBX, a0, a1, a2);
>          break;
>      case INDEX_op_ld16u_i32:
>      case INDEX_op_ld16u_i64:
> -        tcg_out_ldst(s, MO_16, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRH, a0, a1, a2);
>          break;
>      case INDEX_op_ld16s_i32:
> -        tcg_out_ldst(s, MO_16, LDST_LD_S_W, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRSHW, a0, a1, a2);
>          break;
>      case INDEX_op_ld16s_i64:
> -        tcg_out_ldst(s, MO_16, LDST_LD_S_X, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRSHX, a0, a1, a2);
>          break;
>      case INDEX_op_ld_i32:
>      case INDEX_op_ld32u_i64:
> -        tcg_out_ldst(s, MO_32, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRW, a0, a1, a2);
>          break;
>      case INDEX_op_ld32s_i64:
> -        tcg_out_ldst(s, MO_32, LDST_LD_S_X, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRSWX, a0, a1, a2);
>          break;
>      case INDEX_op_ld_i64:
> -        tcg_out_ldst(s, MO_64, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, I3312_LDRX, a0, a1, a2);
>          break;
>  
>      case INDEX_op_st8_i32:
>      case INDEX_op_st8_i64:
> -        tcg_out_ldst(s, MO_8, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, I3312_STRB, REG0(0), a1, a2);
>          break;
>      case INDEX_op_st16_i32:
>      case INDEX_op_st16_i64:
> -        tcg_out_ldst(s, MO_16, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, I3312_STRH, REG0(0), a1, a2);
>          break;
>      case INDEX_op_st_i32:
>      case INDEX_op_st32_i64:
> -        tcg_out_ldst(s, MO_32, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, I3312_STRW, REG0(0), a1, a2);
>          break;
>      case INDEX_op_st_i64:
> -        tcg_out_ldst(s, MO_64, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, I3312_STRX, REG0(0), a1, a2);
>          break;
>  
>      case INDEX_op_add_i32:
> 

Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>