Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32

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

As I mentioned before, I just have one nit with this,
functionally it is fine (and I tested it with multiple targets, so you can add my

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

I describe my nit below:

On 22.09.2014 22:57, Richard Henderson wrote:
> The "old" qemu_ld opcode did not specify the size of the result,
> and so we had to assume full register width.  With the new opcodes,
> we can narrow the result.
> 
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/aarch64/tcg-target.c | 29 ++++++++++++++++-------------
>  tcg/tcg-be-ldst.h        |  1 +
>  2 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tcg/aarch64/tcg-target.c b/tcg/aarch64/tcg-target.c
> index 56dae66..5017cfe 100644
> --- a/tcg/aarch64/tcg-target.c
> +++ b/tcg/aarch64/tcg-target.c
> @@ -1007,7 +1007,7 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      tcg_out_adr(s, TCG_REG_X3, lb->raddr);
>      tcg_out_call(s, qemu_ld_helpers[opc & ~MO_SIGN]);
>      if (opc & MO_SIGN) {
> -        tcg_out_sxt(s, TCG_TYPE_I64, size, lb->datalo_reg, TCG_REG_X0);
> +        tcg_out_sxt(s, lb->type, size, lb->datalo_reg, TCG_REG_X0);
>      } else {
>          tcg_out_mov(s, size == MO_64, lb->datalo_reg, TCG_REG_X0);
>      }
> @@ -1032,14 +1032,15 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>  }
>  
>  static void add_qemu_ldst_label(TCGContext *s, bool is_ld, TCGMemOp opc,
> -                                TCGReg data_reg, TCGReg addr_reg,
> +                                TCGType type, TCGReg data_reg, TCGReg addr_reg,
>                                  int mem_index, tcg_insn_unit *raddr,
>                                  tcg_insn_unit *label_ptr)
>  {
>      TCGLabelQemuLdst *label = new_ldst_label(s);
>  
> -    label->is_ld = is_ld;
>      label->opc = opc;
> +    label->is_ld = is_ld;
> +    label->type = type;
>      label->datalo_reg = data_reg;
>      label->addrlo_reg = addr_reg;
>      label->mem_index = mem_index;
> @@ -1108,7 +1109,7 @@ static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, TCGMemOp s_bits,
>  
>  #endif /* CONFIG_SOFTMMU */
>  
> -static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop,
> +static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop, TCGType type,
>                                     TCGReg data_r, TCGReg addr_r, TCGReg off_r)
>  {
>      const TCGMemOp bswap = memop & MO_BSWAP;
> @@ -1118,7 +1119,8 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop,
>          tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, off_r);
>          break;
>      case MO_SB:
> -        tcg_out_ldst_r(s, I3312_LDRSBX, data_r, addr_r, off_r);
> +        tcg_out_ldst_r(s, type ? I3312_LDRSBX : I3312_LDRSBW,
> +                       data_r, addr_r, off_r);


since we are using the enum type TCGType, why do we check type as "type ?"

I would have expected the conditional to be something like

type == TCG_TYPE_I32 ? I3312_LDRSBW : I3312_LDRSBX

It's pretty obvious what is happening but it might spare someone a lookup into the header file
to test that type 0 is indeed TCG_TYPE_I32.

Ciao,

Claudio

>          break;
>      case MO_UW:
>          tcg_out_ldst_r(s, I3312_LDRH, data_r, addr_r, off_r);
> @@ -1130,9 +1132,10 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop,
>          if (bswap) {
>              tcg_out_ldst_r(s, I3312_LDRH, data_r, addr_r, off_r);
>              tcg_out_rev16(s, data_r, data_r);
> -            tcg_out_sxt(s, TCG_TYPE_I64, MO_16, data_r, data_r);
> +            tcg_out_sxt(s, type, MO_16, data_r, data_r);
>          } else {
> -            tcg_out_ldst_r(s, I3312_LDRSHX, data_r, addr_r, off_r);
> +            tcg_out_ldst_r(s, type ? I3312_LDRSHX : I3312_LDRSHW,
> +                           data_r, addr_r, off_r);
>          }
>          break;
>      case MO_UL:
> @@ -1197,18 +1200,18 @@ static void tcg_out_qemu_st_direct(TCGContext *s, TCGMemOp memop,
>  }
>  
>  static void tcg_out_qemu_ld(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
> -                            TCGMemOp memop, int mem_index)
> +                            TCGMemOp memop, TCGType type, int mem_index)
>  {
>  #ifdef CONFIG_SOFTMMU
>      TCGMemOp s_bits = memop & MO_SIZE;
>      tcg_insn_unit *label_ptr;
>  
>      tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 1);
> -    tcg_out_qemu_ld_direct(s, memop, data_reg, addr_reg, TCG_REG_X1);
> -    add_qemu_ldst_label(s, true, memop, data_reg, addr_reg,
> +    tcg_out_qemu_ld_direct(s, memop, type, data_reg, addr_reg, TCG_REG_X1);
> +    add_qemu_ldst_label(s, true, memop, type, data_reg, addr_reg,
>                          mem_index, s->code_ptr, label_ptr);
>  #else /* !CONFIG_SOFTMMU */
> -    tcg_out_qemu_ld_direct(s, memop, data_reg, addr_reg,
> +    tcg_out_qemu_ld_direct(s, memop, type, data_reg, addr_reg,
>                             GUEST_BASE ? TCG_REG_GUEST_BASE : TCG_REG_XZR);
>  #endif /* CONFIG_SOFTMMU */
>  }
> @@ -1222,7 +1225,7 @@ static void tcg_out_qemu_st(TCGContext *s, TCGReg data_reg, TCGReg addr_reg,
>  
>      tcg_out_tlb_read(s, addr_reg, s_bits, &label_ptr, mem_index, 0);
>      tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg, TCG_REG_X1);
> -    add_qemu_ldst_label(s, false, memop, data_reg, addr_reg,
> +    add_qemu_ldst_label(s, false, memop, s_bits == MO_64, data_reg, addr_reg,
>                          mem_index, s->code_ptr, label_ptr);
>  #else /* !CONFIG_SOFTMMU */
>      tcg_out_qemu_st_direct(s, memop, data_reg, addr_reg,
> @@ -1515,7 +1518,7 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc,
>  
>      case INDEX_op_qemu_ld_i32:
>      case INDEX_op_qemu_ld_i64:
> -        tcg_out_qemu_ld(s, a0, a1, a2, args[3]);
> +        tcg_out_qemu_ld(s, a0, a1, a2, ext, args[3]);
>          break;
>      case INDEX_op_qemu_st_i32:
>      case INDEX_op_qemu_st_i64:
> diff --git a/tcg/tcg-be-ldst.h b/tcg/tcg-be-ldst.h
> index 904eeda..825de14 100644
> --- a/tcg/tcg-be-ldst.h
> +++ b/tcg/tcg-be-ldst.h
> @@ -29,6 +29,7 @@ QEMU_BUILD_BUG_ON(NB_MMU_MODES > 8);
>  typedef struct TCGLabelQemuLdst {
>      TCGMemOp opc : 4;
>      bool is_ld : 1;         /* qemu_ld: true, qemu_st: false */
> +    TCGType type : 1;       /* result type of a load */
>      TCGReg addrlo_reg : 5;  /* reg index for low word of guest virtual addr */
>      TCGReg addrhi_reg : 5;  /* reg index for high word of guest virtual addr */
>      TCGReg datalo_reg : 5;  /* reg index for low word to be loaded or stored */
>