[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL
From: |
Maciej W. Rozycki |
Subject: |
Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only |
Date: |
Mon, 17 Sep 2018 18:10:27 +0100 (BST) |
User-agent: |
Alpine 2.21 (LFD 202 2017-01-01) |
Hi Fredrik,
Nitpicking here, but I think it's what makes code clean and pleasant to
read.
On Sun, 16 Sep 2018, Fredrik Noring wrote:
> diff --git a/target/mips/translate.c b/target/mips/translate.c
> index 77d678353e..327e96307b 100644
> --- a/target/mips/translate.c
> +++ b/target/mips/translate.c
> @@ -22458,6 +22458,7 @@ static void decode_opc_special_legacy(CPUMIPSState
> *env, DisasContext *ctx)
> case OPC_DMULTU:
> case OPC_DDIV:
> case OPC_DDIVU:
> + check_insn_opc_user_only(ctx, INSN_R5900);
> check_insn(ctx, ISA_MIPS3);
> check_mips_64(ctx);
> gen_muldiv(ctx, op1, 0, rs, rt);
I think it would make sense to keep the order of the checks consistent,
or otherwise code starts looking messy.
The predominant order appears to be (as applicable) starting with a
check for the lowest ISA membership, followed by a check for the highest
ISA membership (R6 instruction cuts) and ending with processor-specific
special cases. I think this order makes sense in that it starts with
checks that have a wider scope and then moves on to ones of a narrower
scope, and I think keeping it would be good (as would fixing where the
addition of R6 broke it). Mode checks for otherwise existing
instructions then follow, which complements the pattern.
So please make it:
check_insn(ctx, ISA_MIPS3);
check_insn_opc_user_only(ctx, INSN_R5900);
check_mips_64(ctx);
> @@ -24962,6 +24963,7 @@ static void decode_opc(CPUMIPSState *env,
> DisasContext *ctx)
> break;
> case OPC_LL: /* Load and stores */
> check_insn(ctx, ISA_MIPS2);
> + check_insn_opc_user_only(ctx, INSN_R5900);
> /* Fallthrough */
> case OPC_LWL:
> case OPC_LWR:
> @@ -24987,6 +24989,7 @@ static void decode_opc(CPUMIPSState *env,
> DisasContext *ctx)
> case OPC_SC:
> check_insn(ctx, ISA_MIPS2);
> check_insn_opc_removed(ctx, ISA_MIPS32R6);
> + check_insn_opc_user_only(ctx, INSN_R5900);
> gen_st_cond(ctx, op, rt, rs, imm);
> break;
> case OPC_CACHE:
OK in these two cases (noting a preexisting formatting issue to fix
separately).
> @@ -25253,9 +25256,11 @@ static void decode_opc(CPUMIPSState *env,
> DisasContext *ctx)
>
> #if defined(TARGET_MIPS64)
> /* MIPS64 opcodes */
> + case OPC_LLD:
> + check_insn_opc_user_only(ctx, INSN_R5900);
> + /* fall through */
> case OPC_LDL:
> case OPC_LDR:
> - case OPC_LLD:
> check_insn_opc_removed(ctx, ISA_MIPS32R6);
> /* fall through */
> case OPC_LWU:
And here, because of the fall-through.
> @@ -25275,6 +25280,7 @@ static void decode_opc(CPUMIPSState *env,
> DisasContext *ctx)
> break;
> case OPC_SCD:
> check_insn_opc_removed(ctx, ISA_MIPS32R6);
> + check_insn_opc_user_only(ctx, INSN_R5900);
> check_insn(ctx, ISA_MIPS3);
> check_mips_64(ctx);
> gen_st_cond(ctx, op, rt, rs, imm);
And please make it:
check_insn_opc_removed(ctx, ISA_MIPS32R6);
check_insn(ctx, ISA_MIPS3);
check_insn_opc_user_only(ctx, INSN_R5900);
check_mips_64(ctx);
here (and swapping the two former calls ought to be fixed separately; I
haven't checked if there are more cases like this, but if so, then they
would best be amended with a single change).
Maciej
- [Qemu-devel] [PATCH v4 2/8] target/mips: Support R5900 specific three-operand MULT and MULTU, (continued)
- [Qemu-devel] [PATCH v4 2/8] target/mips: Support R5900 specific three-operand MULT and MULTU, Fredrik Noring, 2018/09/16
- [Qemu-devel] [PATCH v4 3/8] target/mips: Support R5900 instructions MOVN, MOVZ and PREF from MIPS IV, Fredrik Noring, 2018/09/16
- [Qemu-devel] [PATCH v4 4/8] target/mips: Add function to signal RI exception unless user only, Fredrik Noring, 2018/09/16
- [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only, Fredrik Noring, 2018/09/16
- [Qemu-devel] [PATCH v4 6/8] target/mips: Define the R5900 CPU, Fredrik Noring, 2018/09/16
- [Qemu-devel] [PATCH v4 7/8] linux-user/mips: Recognise the R5900 CPU model, Fredrik Noring, 2018/09/16
- [Qemu-devel] [PATCH v4 8/8] elf: Toshiba/Sony rather than MIPS are the implementors of the R5900, Fredrik Noring, 2018/09/16
- Re: [Qemu-devel] [PATCH v4 8/8] elf: Toshiba/Sony rather than MIPS are the implementors of the R5900, Philippe Mathieu-Daudé, 2018/09/16
- Re: [Qemu-devel] [PATCH v4 7/8] linux-user/mips: Recognise the R5900 CPU model, Philippe Mathieu-Daudé, 2018/09/16
- Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only, Philippe Mathieu-Daudé, 2018/09/16
- Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only,
Maciej W. Rozycki <=
- Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only, Fredrik Noring, 2018/09/18
- Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only, Maciej W. Rozycki, 2018/09/18
- Re: [Qemu-devel] [PATCH v4 5/8] target/mips: R5900 DMULT[U], DDIV[U], LL, SC, LLD and SCD are user only, Philippe Mathieu-Daudé, 2018/09/19
- Re: [Qemu-devel] [PATCH v4 4/8] target/mips: Add function to signal RI exception unless user only, Philippe Mathieu-Daudé, 2018/09/16
- Re: [Qemu-devel] [PATCH v4 3/8] target/mips: Support R5900 instructions MOVN, MOVZ and PREF from MIPS IV, Philippe Mathieu-Daudé, 2018/09/16
Re: [Qemu-devel] [PATCH v4 1/8] target/mips: Define R5900 instructions and CPU preprocessor constants, Philippe Mathieu-Daudé, 2018/09/16