Thanks Leon,
I'll make sure future patches are sent inline.
Regards,
Miodrag
________________________________________
From: Leon Alrae
Sent: Friday, January 08, 2016 5:09 PM
To: Miodrag Dinic; Aurelien Jarno
Cc: qemu-devel@nongnu.org; Petar Jovanovic
Subject: Re: [Qemu-devel] [PATCH v2] target-mips: Fix ALIGN instruction when
bp=0
Hi Miodrag,
Thanks for the fix; I've applied it to the target-mips queue (in future
please send patches inline).
Thanks,
Leon
On 04/01/16 15:52, Miodrag Dinic wrote:
> Hello Aurelien,
>
> thanks for your comments and review.
> Version 2 of the patch is in the attachment.
>
> Diff between versions 1 & 2 according to your comments is :
>
> diff --git a/target-mips/translate.c b/target-mips/translate.c
> index f20678c..d2443d3 100644
> --- a/target-mips/translate.c
> +++ b/target-mips/translate.c
> @@ -4632,12 +4632,13 @@ static void gen_align(DisasContext *ctx, int opc, int
> rd, int rs, int rt,
> if (bp == 0) {
> switch (opc) {
> case OPC_ALIGN:
> + tcg_gen_ext32s_tl(cpu_gpr[rd], t0);
> + break;
> #if defined(TARGET_MIPS64)
> - tcg_gen_ext32s_i64(cpu_gpr[rd], t0);
> + case OPC_DALIGN:
> + tcg_gen_mov_tl(cpu_gpr[rd], t0);
> break;
> #endif
> - default:
> - tcg_gen_mov_tl(cpu_gpr[rd], t0);
> }
> } else {
> TCGv t1 = tcg_temp_new();
>
> * As you suggested I used tcg_gen_ext32s_tl() instead of tcg_gen_ext32s_i64()
> for the OPC_ALIGN case.
>
> * I've kept the "TARGET_MIPS64" ifdef guard for the OPC_DALIGN case, to keep
> the change in-line with the rest of the code where this 64-bit instruction
> opcode is used.
>
> Thank you.
>
> Regards,
> Miodrag
>
> ________________________________________
> From: Aurelien Jarno [aurel...@aurel32.net]
> Sent: Friday, January 01, 2016 2:02 PM
> To: Miodrag Dinic
> Cc: qemu-devel@nongnu.org; Petar Jovanovic
> Subject: Re: [PATCH] target-mips: Fix ALIGN instruction when bp=0
>
> [snip]
>
>> From e01539a11061c447bece8dccde1715da9534024d Mon Sep 17 00:00:00 2001
>> From: Miodrag Dinic <miodrag.di...@imgtec.com>
>> Date: Thu, 3 Dec 2015 16:48:57 +0100
>> Subject: [PATCH] target-mips: Fix ALIGN instruction when bp=0
>>
>> If executing ALIGN with shift count bp=0 within mips64 emulation,
>> the result of the operation should be sign extended.
>>
>> Taken from the official documentation (pseudo code) :
>>
>> ALIGN:
>> tmp_rt_hi = unsigned_word(GPR[rt]) << (8*bp)
>> tmp_rs_lo = unsigned_word(GPR[rs]) >> (8*(4-bp))
>> tmp = tmp_rt_hi || tmp_rt_lo
>> GPR[rd] = sign_extend.32(tmp)
>>
>> Signed-off-by: Miodrag Dinic <miodrag.di...@imgtec.com>
>> ---
>> target-mips/translate.c | 10 +++++++++-
>> 1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/target-mips/translate.c b/target-mips/translate.c
>> index 5626647..f20678c 100644
>> --- a/target-mips/translate.c
>> +++ b/target-mips/translate.c
>> @@ -4630,7 +4630,15 @@ static void gen_align(DisasContext *ctx, int opc, int
>> rd, int rs, int rt,
>> t0 = tcg_temp_new();
>> gen_load_gpr(t0, rt);
>> if (bp == 0) {
>> - tcg_gen_mov_tl(cpu_gpr[rd], t0);
>> + switch (opc) {
>> + case OPC_ALIGN:
>> +#if defined(TARGET_MIPS64)
>> + tcg_gen_ext32s_i64(cpu_gpr[rd], t0);
>> + break;
>> +#endif
>
> The way to fix that is basically ok. However you should just use
> tcg_gen_ext32s_tl instead of tcg_gen_ext32s_i64 and drop the
> TARGET_MIPS64 #ifdef.
>
>> + default:
>
> Then you can replace this by OPC_DALIGN for more clarity.
>
>> + tcg_gen_mov_tl(cpu_gpr[rd], t0);
>> + }
>> } else {
>> TCGv t1 = tcg_temp_new();
>> gen_load_gpr(t1, rs);
>
> The resulting binary code should be the same, but less #ifdef means less
> risk of breakage, as the code is always compiled.
>
> --
> Aurelien Jarno GPG: 4096R/1DDD8C9B
> aurel...@aurel32.net http://www.aurel32.net
>
