[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_
From: |
Claudio Fontana |
Subject: |
Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32 |
Date: |
Wed, 24 Sep 2014 10:20:45 +0200 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; rv:24.0) Gecko/20100101 Thunderbird/24.0.1 |
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 <address@hidden>
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 <address@hidden>
> ---
> 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
> */
>
- [Qemu-devel] [PULL 07/11] tcg: Compress TCGLabelQemuLdst, (continued)
[Qemu-devel] [PULL 06/11] tcg-sparc: Use UMULXHI instruction, Richard Henderson, 2014/09/22
[Qemu-devel] [PULL 08/11] tcg: Move TCG_TYPE_COUNT out of enum TCGType, Richard Henderson, 2014/09/22
[Qemu-devel] [PULL 10/11] qemu/compiler: Define QEMU_ARTIFICIAL, Richard Henderson, 2014/09/22
[Qemu-devel] [PULL 11/11] tcg: Always enable TCGv type checking, Richard Henderson, 2014/09/22
[Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32, Richard Henderson, 2014/09/22
- Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32,
Claudio Fontana <=