Re: [Qemu-devel] [PATCH v3 23/26] tcg-aarch64: Replace aarch64_ldst_op_data with TCGMemOp

[Claudio Fontana]

On 03.04.2014 21:56, Richard Henderson wrote:
> The definition of op_data included opcode bits, not just
> the size field of the various ldst instructions.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/aarch64/tcg-target.c | 111 
> +++++++++++++++++++++--------------------------
>  1 file changed, 49 insertions(+), 62 deletions(-)
> 
> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c
> index 5ecc20c..9a2e4a6 100644
> --- a/tcg/aarch64/tcg-target.c
> +++ b/tcg/aarch64/tcg-target.c
> @@ -242,13 +242,6 @@ static const enum aarch64_cond_code 
> tcg_cond_to_aarch64[] = {
>      [TCG_COND_LEU] = COND_LS,
>  };
>  
> -/* opcodes for LDR / STR instructions with base + simm9 addressing */
> -enum aarch64_ldst_op_data { /* size of the data moved */
> -    LDST_8 = 0x38,
> -    LDST_16 = 0x78,
> -    LDST_32 = 0xb8,
> -    LDST_64 = 0xf8,
> -};
>  enum aarch64_ldst_op_type { /* type of operation */
>      LDST_ST = 0x0,    /* store */
>      LDST_LD = 0x4,    /* load */
> @@ -490,25 +483,23 @@ static void tcg_out_insn_3509(TCGContext *s, 
> AArch64Insn insn, TCGType ext,
>  }
>  
>  
> -static inline void tcg_out_ldst_9(TCGContext *s,
> -                                  enum aarch64_ldst_op_data op_data,
> +static inline void tcg_out_ldst_9(TCGContext *s, TCGMemOp size,
>                                    enum aarch64_ldst_op_type op_type,
>                                    TCGReg rd, TCGReg rn, intptr_t offset)
>  {
>      /* use LDUR with BASE register with 9bit signed unscaled offset */
> -    tcg_out32(s, op_data << 24 | op_type << 20
> +    tcg_out32(s, 0x38000000 | size << 30 | op_type << 20
>                | (offset & 0x1ff) << 12 | rn << 5 | rd);
>  }
>  
>  /* tcg_out_ldst_12 expects a scaled unsigned immediate offset */
> -static inline void tcg_out_ldst_12(TCGContext *s,
> -                                   enum aarch64_ldst_op_data op_data,
> +static inline void tcg_out_ldst_12(TCGContext *s, TCGMemOp size,
>                                     enum aarch64_ldst_op_type op_type,
>                                     TCGReg rd, TCGReg rn,
>                                     tcg_target_ulong scaled_uimm)
>  {
> -    tcg_out32(s, (op_data | 1) << 24
> -              | op_type << 20 | scaled_uimm << 10 | rn << 5 | rd);
> +    tcg_out32(s, 0x39000000 | size << 30 | op_type << 20
> +              | scaled_uimm << 10 | rn << 5 | rd);
>  }
>  
>  /* Register to register move using ORR (shifted register with no shift). */
> @@ -646,44 +637,40 @@ static void tcg_out_movi(TCGContext *s, TCGType type, 
> TCGReg rd,
>      }
>  }
>  
> -static inline void tcg_out_ldst_r(TCGContext *s,
> -                                  enum aarch64_ldst_op_data op_data,
> +static inline void tcg_out_ldst_r(TCGContext *s, TCGMemOp size,
>                                    enum aarch64_ldst_op_type op_type,
>                                    TCGReg rd, TCGReg base, TCGReg regoff)
>  {
>      /* load from memory to register using base + 64bit register offset */
>      /* using f.e. STR Wt, [Xn, Xm] 0xb8600800|(regoff << 16)|(base << 5)|rd 
> */
>      /* the 0x6000 is for the "no extend field" */
> -    tcg_out32(s, 0x00206800
> -              | op_data << 24 | op_type << 20 | regoff << 16 | base << 5 | 
> rd);
> +    tcg_out32(s, 0x38206800 | size << 30 | op_type << 20
> +              | regoff << 16 | base << 5 | rd);
>  }
>  
>  /* solve the whole ldst problem */
> -static inline void tcg_out_ldst(TCGContext *s, enum aarch64_ldst_op_data 
> data,
> +static inline void tcg_out_ldst(TCGContext *s, TCGMemOp size,
>                                  enum aarch64_ldst_op_type type,
>                                  TCGReg rd, TCGReg rn, intptr_t offset)
>  {
>      if (offset >= -256 && offset < 256) {
> -        tcg_out_ldst_9(s, data, type, rd, rn, offset);
> +        tcg_out_ldst_9(s, size, type, rd, rn, offset);
>          return;
>      }
>  
> -    if (offset >= 256) {
> -        /* if the offset is naturally aligned and in range,
> -           then we can use the scaled uimm12 encoding */
> -        unsigned int s_bits = data >> 6;
> -        if (!(offset & ((1 << s_bits) - 1))) {
> -            tcg_target_ulong scaled_uimm = offset >> s_bits;
> -            if (scaled_uimm <= 0xfff) {
> -                tcg_out_ldst_12(s, data, type, rd, rn, scaled_uimm);
> -                return;
> -            }
> +    /* 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;
> +        if (scaled_uimm <= 0xfff) {
> +            tcg_out_ldst_12(s, size, type, rd, rn, scaled_uimm);
> +            return;
>          }
>      }
>  
> -    /* worst-case scenario, move offset to temp register, use reg offset */
> +    /* 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, data, type, rd, rn, TCG_REG_TMP);
> +    tcg_out_ldst_r(s, size, type, rd, rn, TCG_REG_TMP);
>  }
>  
>  static inline void tcg_out_mov(TCGContext *s,
> @@ -697,14 +684,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) ? LDST_64 : LDST_32, LDST_LD,
> +    tcg_out_ldst(s, type == TCG_TYPE_I64 ? MO_64 : MO_32, LDST_LD,
>                   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) ? LDST_64 : LDST_32, LDST_ST,
> +    tcg_out_ldst(s, type == TCG_TYPE_I64 ? MO_64 : MO_32, LDST_ST,
>                   arg, arg1, arg2);
>  }
>  
> @@ -1104,12 +1091,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 ? LDST_64 : LDST_32,
> +    tcg_out_ldst(s, TARGET_LONG_BITS == 64 ? MO_64 : MO_32,
>                   LDST_LD, 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, LDST_64, LDST_LD, TCG_REG_X1, TCG_REG_X2,
> +    tcg_out_ldst(s, MO_64, LDST_LD, TCG_REG_X1, TCG_REG_X2,
>                   (tlb_offset & 0xfff) + (offsetof(CPUTLBEntry, addend)) -
>                   (is_read ? offsetof(CPUTLBEntry, addr_read)
>                    : offsetof(CPUTLBEntry, addr_write)));
> @@ -1131,43 +1118,43 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, 
> TCGMemOp memop,
>  
>      switch (memop & MO_SSIZE) {
>      case MO_UB:
> -        tcg_out_ldst_r(s, LDST_8, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_8, LDST_LD, data_r, addr_r, off_r);
>          break;
>      case MO_SB:
> -        tcg_out_ldst_r(s, LDST_8, LDST_LD_S_X, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_8, LDST_LD_S_X, data_r, addr_r, off_r);
>          break;
>      case MO_UW:
> -        tcg_out_ldst_r(s, LDST_16, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_16, LDST_LD, 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, LDST_16, LDST_LD, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, MO_16, LDST_LD, 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, LDST_16, LDST_LD_S_X, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, MO_16, LDST_LD_S_X, data_r, addr_r, off_r);
>          }
>          break;
>      case MO_UL:
> -        tcg_out_ldst_r(s, LDST_32, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_32, LDST_LD, 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, LDST_32, LDST_LD, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, MO_32, LDST_LD, 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, LDST_32, LDST_LD_S_X, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, MO_32, LDST_LD_S_X, data_r, addr_r, off_r);
>          }
>          break;
>      case MO_Q:
> -        tcg_out_ldst_r(s, LDST_64, LDST_LD, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_64, LDST_LD, data_r, addr_r, off_r);
>          if (bswap) {
>              tcg_out_rev(s, TCG_TYPE_I64, data_r, data_r);
>          }
> @@ -1184,28 +1171,28 @@ static void tcg_out_qemu_st_direct(TCGContext *s, 
> TCGMemOp memop,
>  
>      switch (memop & MO_SIZE) {
>      case MO_8:
> -        tcg_out_ldst_r(s, LDST_8, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_8, LDST_ST, 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, LDST_16, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_16, LDST_ST, 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, LDST_32, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_32, LDST_ST, 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, LDST_64, LDST_ST, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, MO_64, LDST_ST, data_r, addr_r, off_r);
>          break;
>      default:
>          tcg_abort();
> @@ -1298,49 +1285,49 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>  
>      case INDEX_op_ld8u_i32:
>      case INDEX_op_ld8u_i64:
> -        tcg_out_ldst(s, LDST_8, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, MO_8, LDST_LD, a0, a1, a2);
>          break;
>      case INDEX_op_ld8s_i32:
> -        tcg_out_ldst(s, LDST_8, LDST_LD_S_W, a0, a1, a2);
> +        tcg_out_ldst(s, MO_8, LDST_LD_S_W, a0, a1, a2);
>          break;
>      case INDEX_op_ld8s_i64:
> -        tcg_out_ldst(s, LDST_8, LDST_LD_S_X, a0, a1, a2);
> +        tcg_out_ldst(s, MO_8, LDST_LD_S_X, a0, a1, a2);
>          break;
>      case INDEX_op_ld16u_i32:
>      case INDEX_op_ld16u_i64:
> -        tcg_out_ldst(s, LDST_16, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, MO_16, LDST_LD, a0, a1, a2);
>          break;
>      case INDEX_op_ld16s_i32:
> -        tcg_out_ldst(s, LDST_16, LDST_LD_S_W, a0, a1, a2);
> +        tcg_out_ldst(s, MO_16, LDST_LD_S_W, a0, a1, a2);
>          break;
>      case INDEX_op_ld16s_i64:
> -        tcg_out_ldst(s, LDST_16, LDST_LD_S_X, a0, a1, a2);
> +        tcg_out_ldst(s, MO_16, LDST_LD_S_X, a0, a1, a2);
>          break;
>      case INDEX_op_ld_i32:
>      case INDEX_op_ld32u_i64:
> -        tcg_out_ldst(s, LDST_32, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, MO_32, LDST_LD, a0, a1, a2);
>          break;
>      case INDEX_op_ld32s_i64:
> -        tcg_out_ldst(s, LDST_32, LDST_LD_S_X, a0, a1, a2);
> +        tcg_out_ldst(s, MO_32, LDST_LD_S_X, a0, a1, a2);
>          break;
>      case INDEX_op_ld_i64:
> -        tcg_out_ldst(s, LDST_64, LDST_LD, a0, a1, a2);
> +        tcg_out_ldst(s, MO_64, LDST_LD, a0, a1, a2);
>          break;
>  
>      case INDEX_op_st8_i32:
>      case INDEX_op_st8_i64:
> -        tcg_out_ldst(s, LDST_8, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, MO_8, LDST_ST, REG0(0), a1, a2);
>          break;
>      case INDEX_op_st16_i32:
>      case INDEX_op_st16_i64:
> -        tcg_out_ldst(s, LDST_16, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, MO_16, LDST_ST, REG0(0), a1, a2);
>          break;
>      case INDEX_op_st_i32:
>      case INDEX_op_st32_i64:
> -        tcg_out_ldst(s, LDST_32, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, MO_32, LDST_ST, REG0(0), a1, a2);
>          break;
>      case INDEX_op_st_i64:
> -        tcg_out_ldst(s, LDST_64, LDST_ST, REG0(0), a1, a2);
> +        tcg_out_ldst(s, MO_64, LDST_ST, REG0(0), a1, a2);
>          break;
>  
>      case INDEX_op_add_i32:
> 

Reviewed-by: Claudio Fontana <address@hidden>