* [RFC PATCH] target/sh4: Fix SUBV opcode
@ 2024-04-30 12:06 Philippe Mathieu-Daudé
2024-04-30 13:10 ` Paul Cercueil
2024-04-30 14:16 ` Paul Cercueil
0 siblings, 2 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-04-30 12:06 UTC (permalink / raw)
To: qemu-devel
Cc: John Paul Adrian Glaubitz, Yoshinori Sato,
Philippe Mathieu-Daudé, qemu-stable, Paul Cercueil
The documentation says:
SUBV Rm, Rn Rn - Rm -> Rn, underflow -> T
While correctly performing the substraction, the underflow
is not detected.
While we can check the high xored bit for overflow, for
underflow we need to check the xored value is not negative.
Cc: qemu-stable@nongnu.org
Fixes: ad8d25a11f ("target-sh4: implement addv and subv using TCG")
Reported-by: Paul Cercueil <paul@crapouillou.net>
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2318
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
target/sh4/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index 4a1dd0d1f4..1c48d8ebea 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -937,7 +937,7 @@ static void _decode_opc(DisasContext * ctx)
t2 = tcg_temp_new();
tcg_gen_xor_i32(t2, REG(B11_8), REG(B7_4));
tcg_gen_and_i32(t1, t1, t2);
- tcg_gen_shri_i32(cpu_sr_t, t1, 31);
+ tcg_gen_setcondi_i32(TCG_COND_GE, cpu_sr_t, t1, 0);
tcg_gen_mov_i32(REG(B11_8), t0);
}
return;
--
2.41.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [RFC PATCH] target/sh4: Fix SUBV opcode
2024-04-30 12:06 [RFC PATCH] target/sh4: Fix SUBV opcode Philippe Mathieu-Daudé
@ 2024-04-30 13:10 ` Paul Cercueil
2024-04-30 14:16 ` Paul Cercueil
1 sibling, 0 replies; 4+ messages in thread
From: Paul Cercueil @ 2024-04-30 13:10 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: John Paul Adrian Glaubitz, Yoshinori Sato, qemu-stable
Hi Philippe,
Le mardi 30 avril 2024 à 14:06 +0200, Philippe Mathieu-Daudé a écrit :
> The documentation says:
>
> SUBV Rm, Rn Rn - Rm -> Rn, underflow -> T
>
> While correctly performing the substraction, the underflow
> is not detected.
>
> While we can check the high xored bit for overflow, for
> underflow we need to check the xored value is not negative.
This fix still does not work properly; it will incorrectly set the
underflow bit when the sign changes from positive to negative.
e.g. Rn == 0 and Rm == 2, the result will be Rn == -2, without any
underflow.
Cheers,
-Paul
>
> Cc: qemu-stable@nongnu.org
> Fixes: ad8d25a11f ("target-sh4: implement addv and subv using TCG")
> Reported-by: Paul Cercueil <paul@crapouillou.net>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2318
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> target/sh4/translate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 4a1dd0d1f4..1c48d8ebea 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -937,7 +937,7 @@ static void _decode_opc(DisasContext * ctx)
> t2 = tcg_temp_new();
> tcg_gen_xor_i32(t2, REG(B11_8), REG(B7_4));
> tcg_gen_and_i32(t1, t1, t2);
> - tcg_gen_shri_i32(cpu_sr_t, t1, 31);
> + tcg_gen_setcondi_i32(TCG_COND_GE, cpu_sr_t, t1, 0);
> tcg_gen_mov_i32(REG(B11_8), t0);
> }
> return;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] target/sh4: Fix SUBV opcode
2024-04-30 12:06 [RFC PATCH] target/sh4: Fix SUBV opcode Philippe Mathieu-Daudé
2024-04-30 13:10 ` Paul Cercueil
@ 2024-04-30 14:16 ` Paul Cercueil
2024-04-30 14:32 ` Philippe Mathieu-Daudé
1 sibling, 1 reply; 4+ messages in thread
From: Paul Cercueil @ 2024-04-30 14:16 UTC (permalink / raw)
To: Philippe Mathieu-Daudé, qemu-devel
Cc: John Paul Adrian Glaubitz, Yoshinori Sato, qemu-stable
Hi Philippe,
If I'm not mistaken, the overflow / underflow can be calculated like
this:
T = ((Rn ^ Rm) & (Result ^ Rn)) >> 31
Looking at what Qemu does (before this patch), it was doing this:
T = ((Rn ^ Rm) & (Result ^ Rm)) >> 31
I changed line 936 to this, and overflow / underflow with SUBV now seem
to work fine:
tcg_gen_xor_i32(t1, t0, REG(B11_8));
So a change from REG(B7_B4) to REG(B11_8).
Cheers,
-Paul
Le mardi 30 avril 2024 à 14:06 +0200, Philippe Mathieu-Daudé a écrit :
> The documentation says:
>
> SUBV Rm, Rn Rn - Rm -> Rn, underflow -> T
>
> While correctly performing the substraction, the underflow
> is not detected.
>
> While we can check the high xored bit for overflow, for
> underflow we need to check the xored value is not negative.
>
> Cc: qemu-stable@nongnu.org
> Fixes: ad8d25a11f ("target-sh4: implement addv and subv using TCG")
> Reported-by: Paul Cercueil <paul@crapouillou.net>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2318
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
> target/sh4/translate.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 4a1dd0d1f4..1c48d8ebea 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -937,7 +937,7 @@ static void _decode_opc(DisasContext * ctx)
> t2 = tcg_temp_new();
> tcg_gen_xor_i32(t2, REG(B11_8), REG(B7_4));
> tcg_gen_and_i32(t1, t1, t2);
> - tcg_gen_shri_i32(cpu_sr_t, t1, 31);
> + tcg_gen_setcondi_i32(TCG_COND_GE, cpu_sr_t, t1, 0);
> tcg_gen_mov_i32(REG(B11_8), t0);
> }
> return;
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [RFC PATCH] target/sh4: Fix SUBV opcode
2024-04-30 14:16 ` Paul Cercueil
@ 2024-04-30 14:32 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 4+ messages in thread
From: Philippe Mathieu-Daudé @ 2024-04-30 14:32 UTC (permalink / raw)
To: Paul Cercueil, qemu-devel
Cc: John Paul Adrian Glaubitz, Yoshinori Sato, qemu-stable
On 30/4/24 16:16, Paul Cercueil wrote:
> Hi Philippe,
>
> If I'm not mistaken, the overflow / underflow can be calculated like
> this:
>
> T = ((Rn ^ Rm) & (Result ^ Rn)) >> 31
>
> Looking at what Qemu does (before this patch), it was doing this:
> T = ((Rn ^ Rm) & (Result ^ Rm)) >> 31
>
> I changed line 936 to this, and overflow / underflow with SUBV now seem
> to work fine:
>
> tcg_gen_xor_i32(t1, t0, REG(B11_8));
>
> So a change from REG(B7_B4) to REG(B11_8).
Correct, thanks!
>
> Cheers,
> -Paul
>
> Le mardi 30 avril 2024 à 14:06 +0200, Philippe Mathieu-Daudé a écrit :
>> The documentation says:
>>
>> SUBV Rm, Rn Rn - Rm -> Rn, underflow -> T
>>
>> While correctly performing the substraction, the underflow
>> is not detected.
>>
>> While we can check the high xored bit for overflow, for
>> underflow we need to check the xored value is not negative.
>>
>> Cc: qemu-stable@nongnu.org
>> Fixes: ad8d25a11f ("target-sh4: implement addv and subv using TCG")
>> Reported-by: Paul Cercueil <paul@crapouillou.net>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2318
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>> target/sh4/translate.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
>> index 4a1dd0d1f4..1c48d8ebea 100644
>> --- a/target/sh4/translate.c
>> +++ b/target/sh4/translate.c
>> @@ -937,7 +937,7 @@ static void _decode_opc(DisasContext * ctx)
>> t2 = tcg_temp_new();
>> tcg_gen_xor_i32(t2, REG(B11_8), REG(B7_4));
>> tcg_gen_and_i32(t1, t1, t2);
>> - tcg_gen_shri_i32(cpu_sr_t, t1, 31);
>> + tcg_gen_setcondi_i32(TCG_COND_GE, cpu_sr_t, t1, 0);
>> tcg_gen_mov_i32(REG(B11_8), t0);
>> }
>> return;
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-04-30 14:35 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 12:06 [RFC PATCH] target/sh4: Fix SUBV opcode Philippe Mathieu-Daudé
2024-04-30 13:10 ` Paul Cercueil
2024-04-30 14:16 ` Paul Cercueil
2024-04-30 14:32 ` Philippe Mathieu-Daudé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).