On Wed, 26 Aug 2026 17:36:22 +0900,
Oleg Endo wrote:
>
> [1 <text/plain; UTF-8 (quoted-printable)>]
> Hi!
>
> It seems the implementation of the mac.w / mac.l insn is wrong:
>
> mac.w @Rm+,@Rn+
> mac.l @Rm+,@Rn+
>
> Qemu's implementation first reads both mems, then applies the post-inc.
> However,
>
> mac.w @R0+,@R0+
>
> should actually do:
> read @(R0 + 0)
> R0 += 2
> read @(R0 + 2)
> R0 += 2
>
> That's also how GDB sh-sim implements it.
> The attached patch fixes it.
>
> I ran into this issue recently when trying to use qemu for sh4.
> I'm not on the list, so please add me in CC in the reply.
>
> Best regards,
> Oleg Endo
> [2 qemu-sh4-mac-same-reg.patch <text/x-patch; UTF-8 (base64)>]
> diff --git a/target/sh4/translate.c b/target/sh4/translate.c
> index 53b0921..efbc8ab 100644
> --- a/target/sh4/translate.c
> +++ b/target/sh4/translate.c
> @@ -809,8 +809,17 @@ static void _decode_opc(DisasContext * ctx)
> tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
> MO_TESL | MO_ALIGN);
> arg1 = tcg_temp_new();
> - tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
> - MO_TESL | MO_ALIGN);
> + if (B7_4 == B11_8) {
> + /* When Rm and Rn are the same register, the second
> + operand is read after the first post-increment. */
> + TCGv addr = tcg_temp_new();
> + tcg_gen_addi_i32(addr, REG(B11_8), 4);
> + tcg_gen_qemu_ld_i32(arg1, addr, ctx->memidx,
> + MO_TESL | MO_ALIGN);
> + } else {
> + tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
> + MO_TESL | MO_ALIGN);
> + }
> gen_helper_macl(tcg_env, arg0, arg1);
> tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 4);
> tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
> @@ -823,8 +832,16 @@ static void _decode_opc(DisasContext * ctx)
> tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
> MO_TESW | MO_ALIGN);
> arg1 = tcg_temp_new();
> - tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
> - MO_TESW | MO_ALIGN);
> + if (B7_4 == B11_8) {
> + /* See mac.l above. */
> + TCGv addr = tcg_temp_new();
> + tcg_gen_addi_i32(addr, REG(B11_8), 2);
> + tcg_gen_qemu_ld_i32(arg1, addr, ctx->memidx,
> + MO_TESW | MO_ALIGN);
> + } else {
> + tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
> + MO_TESW | MO_ALIGN);
> + }
> gen_helper_macw(tcg_env, arg0, arg1);
> tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 2);
> tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 2);
It is true that the current behavior differs from the pseudocode in the manual.
Wouldn't it be simpler to just generate a TCG that matches the pseudocode?
I'm sorry, I haven't actually tested this change yet.
diff --git a/target/sh4/translate.c b/target/sh4/translate.c
index e503a97b22..64fe2cd31d 100644
--- a/target/sh4/translate.c
+++ b/target/sh4/translate.c
@@ -801,13 +801,13 @@ static void _decode_opc(DisasContext * ctx)
{
TCGv arg0, arg1;
arg0 = tcg_temp_new();
- tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
- MO_TESL | MO_ALIGN);
- tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 4);
arg1 = tcg_temp_new();
tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
MO_TESL | MO_ALIGN);
tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 4);
+ tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
+ MO_TESL | MO_ALIGN);
+ tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 4);
gen_helper_macl(tcg_env, arg0, arg1);
}
return;
@@ -815,13 +815,13 @@ static void _decode_opc(DisasContext * ctx)
{
TCGv arg0, arg1;
arg0 = tcg_temp_new();
- tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
- MO_TESW | MO_ALIGN);
- tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 2);
arg1 = tcg_temp_new();
tcg_gen_qemu_ld_i32(arg1, REG(B11_8), ctx->memidx,
MO_TESW | MO_ALIGN);
tcg_gen_addi_i32(REG(B11_8), REG(B11_8), 2);
+ tcg_gen_qemu_ld_i32(arg0, REG(B7_4), ctx->memidx,
+ MO_TESW | MO_ALIGN);
+ tcg_gen_addi_i32(REG(B7_4), REG(B7_4), 2);
gen_helper_macw(tcg_env, arg0, arg1);
}
return;
--
Yosinori Sato
