[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x
From: |
Mark Cave-Ayland |
Subject: |
Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x |
Date: |
Tue, 7 May 2019 19:04:42 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
On 07/05/2019 06:28, David Gibson wrote:
> On Tue, May 07, 2019 at 10:48:06AM +1000, Anton Blanchard wrote:
>> During the conversion these instructions were incorrectly treated as
>> stores. We need to use set_cpu_vsr* and not get_cpu_vsr*.
>>
>> Fixes: 8b3b2d75c7c0 ("introduce get_cpu_vsr{l,h}() and set_cpu_vsr{l,h}()
>> helpers for VSR register access")
>> Signed-off-by: Anton Blanchard <address@hidden>
>> ---
>> target/ppc/translate/vsx-impl.inc.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/target/ppc/translate/vsx-impl.inc.c
>> b/target/ppc/translate/vsx-impl.inc.c
>> index 05b75105be..c13f84e745 100644
>> --- a/target/ppc/translate/vsx-impl.inc.c
>> +++ b/target/ppc/translate/vsx-impl.inc.c
>> @@ -102,8 +102,7 @@ static void gen_lxvw4x(DisasContext *ctx)
>> }
>> xth = tcg_temp_new_i64();
>> xtl = tcg_temp_new_i64();
>> - get_cpu_vsrh(xth, xT(ctx->opcode));
>> - get_cpu_vsrl(xtl, xT(ctx->opcode));
>> +
>
> Something seems amiss here. Clearly we do need a set..() back to the
> loaded register, but with the removal of these gets, it doesn't look
> like the xth and xtl temporaries are initialized any more.
>
>> gen_set_access_type(ctx, ACCESS_INT);
>> EA = tcg_temp_new();
>>
>> @@ -126,6 +125,8 @@ static void gen_lxvw4x(DisasContext *ctx)
>> tcg_gen_addi_tl(EA, EA, 8);
>> tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
>> }
>> + set_cpu_vsrh(xT(ctx->opcode), xth);
>> + set_cpu_vsrl(xT(ctx->opcode), xtl);
>> tcg_temp_free(EA);
>> tcg_temp_free_i64(xth);
>> tcg_temp_free_i64(xtl);
>> @@ -185,8 +186,6 @@ static void gen_lxvh8x(DisasContext *ctx)
>> }
>> xth = tcg_temp_new_i64();
>> xtl = tcg_temp_new_i64();
>> - get_cpu_vsrh(xth, xT(ctx->opcode));
>> - get_cpu_vsrl(xtl, xT(ctx->opcode));
>> gen_set_access_type(ctx, ACCESS_INT);
>>
>> EA = tcg_temp_new();
>> @@ -197,6 +196,8 @@ static void gen_lxvh8x(DisasContext *ctx)
>> if (ctx->le_mode) {
>> gen_bswap16x8(xth, xtl, xth, xtl);
>> }
>> + set_cpu_vsrh(xT(ctx->opcode), xth);
>> + set_cpu_vsrl(xT(ctx->opcode), xtl);
>> tcg_temp_free(EA);
>> tcg_temp_free_i64(xth);
>> tcg_temp_free_i64(xtl);
>> @@ -214,14 +215,14 @@ static void gen_lxvb16x(DisasContext *ctx)
>> }
>> xth = tcg_temp_new_i64();
>> xtl = tcg_temp_new_i64();
>> - get_cpu_vsrh(xth, xT(ctx->opcode));
>> - get_cpu_vsrl(xtl, xT(ctx->opcode));
>> gen_set_access_type(ctx, ACCESS_INT);
>> EA = tcg_temp_new();
>> gen_addr_reg_index(ctx, EA);
>> tcg_gen_qemu_ld_i64(xth, EA, ctx->mem_idx, MO_BEQ);
>> tcg_gen_addi_tl(EA, EA, 8);
>> tcg_gen_qemu_ld_i64(xtl, EA, ctx->mem_idx, MO_BEQ);
>> + set_cpu_vsrh(xT(ctx->opcode), xth);
>> + set_cpu_vsrl(xT(ctx->opcode), xtl);
>> tcg_temp_free(EA);
>> tcg_temp_free_i64(xth);
>> tcg_temp_free_i64(xtl);
AFAICT I think that this is correct since the patterns should be as follows:
Load instructions:
tcg_gen_qemu_ld_i64(xth, ...);
set_cpu_vsrh(n, xth);
Store instructions:
get_cpu_vsrh(xth, n);
tcg_gen_qemu_st_i64(xth, ...);
I remember that when I first started experimenting with the very first version
of
this patchset last year, someone on IRC (maybe Richard?) pointed out that I had
inverted the load and store operations and so I went and reworked them all from
scratch. Unfortunately with this and Greg's patch for stxsdx I have a feeling
that
something when wrong during a cherry-pick or rebase of the patchset :(
Following on from this I've just gone through the load/store operations once
again
and spotted two things:
1) VSX_LOAD_SCALAR_DS has an extra get_cpu_vsrh() which can be removed
diff --git a/target/ppc/translate/vsx-impl.inc.c
b/target/ppc/translate/vsx-impl.inc.c
index 11d9b75d01..004ea56c4f 100644
--- a/target/ppc/translate/vsx-impl.inc.c
+++ b/target/ppc/translate/vsx-impl.inc.c
@@ -329,7 +329,6 @@ static void gen_##name(DisasContext *ctx)
\
return; \
} \
xth = tcg_temp_new_i64(); \
- get_cpu_vsrh(xth, rD(ctx->opcode) + 32); \
gen_set_access_type(ctx, ACCESS_INT); \
EA = tcg_temp_new(); \
gen_addr_imm_index(ctx, EA, 0x03); \
2) VSX_VECTOR_LOAD_STORE is confusing and should be split into separate
VSX_VECTOR_LOAD and VSX_VECTOR_STORE macros
Does that sound reasonable? I'm also thinking that we should consider adding a
CC to
stable for patches 4, 5 and 9 in this series since these are genuine
regressions.
ATB,
Mark.
- Re: [Qemu-devel] [PATCH v2] target/ppc: Fix xvabs[sd]p, xvnabs[sd]p, xvneg[sd]p, xvcpsgn[sd]p, (continued)
- [Qemu-devel] [PATCH 3/9] target/ppc: Fix xxbrq, xxbrw, Anton Blanchard, 2019/05/06
- [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, Anton Blanchard, 2019/05/06
Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, Mark Cave-Ayland, 2019/05/10
- Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, Anton Blanchard, 2019/05/21
- Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, David Gibson, 2019/05/21
- Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, Mark Cave-Ayland, 2019/05/22
- Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, David Gibson, 2019/05/22
- Re: [Qemu-devel] [PATCH 4/9] target/ppc: Fix lxvw4x, lxvh8x and lxvb16x, Mark Cave-Ayland, 2019/05/24