[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-2.5 17/30] m68k: ori/andi/subi/addi/eori/cmp
From: |
Richard Henderson |
Subject: |
Re: [Qemu-devel] [PATCH for-2.5 17/30] m68k: ori/andi/subi/addi/eori/cmpi can modify SR/CCR |
Date: |
Wed, 12 Aug 2015 09:44:37 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.1.0 |
On 08/09/2015 01:13 PM, Laurent Vivier wrote:
> Signed-off-by: Laurent Vivier <address@hidden>
> ---
> target-m68k/translate.c | 95
> ++++++++++++++++++++++++++++++-------------------
> 1 file changed, 58 insertions(+), 37 deletions(-)
>
> diff --git a/target-m68k/translate.c b/target-m68k/translate.c
> index 6a426e1..9e379b3 100644
> --- a/target-m68k/translate.c
> +++ b/target-m68k/translate.c
> @@ -1343,6 +1343,42 @@ DISAS_INSN(bitop_im)
> DEST_EA(env, insn, opsize, tmp, &addr);
> }
> }
> +
> +static TCGv gen_get_ccr(DisasContext *s)
> +{
> + TCGv dest;
> +
> + gen_flush_flags(s);
> + dest = tcg_temp_new();
> + tcg_gen_shli_i32(dest, QREG_CC_X, 4);
> + tcg_gen_or_i32(dest, dest, QREG_CC_DEST);
> + return dest;
> +}
> +
> +static TCGv gen_get_sr(DisasContext *s)
> +{
> + TCGv ccr;
> + TCGv sr;
> +
> + ccr = gen_get_ccr(s);
> + sr = tcg_temp_new();
> + tcg_gen_andi_i32(sr, QREG_SR, 0xffe0);
> + tcg_gen_or_i32(sr, sr, ccr);
> + return sr;
> +}
Leaking the temporary produced by gen_get_ccr.
> +
> +static void gen_set_sr(DisasContext *s, TCGv val, int ccr_only)
> +{
> + TCGv tmp;
> + tmp = tcg_temp_new();
> + tcg_gen_andi_i32(QREG_CC_DEST, val, 0xf);
> + tcg_gen_shri_i32(tmp, val, 4);
> + tcg_gen_andi_i32(QREG_CC_X, tmp, 1);
> + if (!ccr_only) {
> + gen_helper_set_sr(cpu_env, val);
> + }
> +}
Leaking tmp. And you don't even need to allocate it -- perform the shift into
QREG_CC_X.
> +
> DISAS_INSN(arith_im)
> {
> int op;
> @@ -1367,7 +1403,20 @@ DISAS_INSN(arith_im)
> default:
> abort();
> }
> - SRC_EA(env, src1, opsize, -1, (op == 6) ? NULL : &addr);
> + if ((op == 0 || op == 1) &&
The subject line is surely misleading, as this is only ANDI/ORI, right? Again,
some more commentary would be helpful.
> + (insn & 0x3f) == 0x3c) {
> + if (opsize == OS_BYTE) {
> + src1 = gen_get_ccr(s);
> + } else {
> + if (IS_USER(s)) {
> + gen_exception(s, s->pc - 2, EXCP_PRIVILEGE);
> + return;
> + }
> + src1 = gen_get_sr(s);
> + }
> + } else {
> + SRC_EA(env, src1, opsize, -1, (op == 6) ? NULL : &addr);
> + }
> dest = tcg_temp_new();
> switch (op) {
> case 0: /* ori */
> @@ -1405,7 +1454,14 @@ DISAS_INSN(arith_im)
> default:
> abort();
> }
> - DEST_EA(env, insn, opsize, dest, &addr);
> + if (op != 6) {
> + if ((op == 0 || op == 1) &&
> + (insn & 0x3f) == 0x3c) {
> + gen_set_sr(s, dest, opsize == OS_BYTE);
> + } else {
> + DEST_EA(env, insn, opsize, dest, &addr);
> + }
> + }
That said, I think this code should be rearranged so that you don't have to
replicate so many conditionals. In particular, the only thing of use in the
middle of import for the ccr insns are two lines: tcg_gen_andi/ori_tl.
I think it would be better to structure as
if ((insn & 0x3f) == 0x3c && (op == 0 || op == 1)) {
if (opsize == OS_BYTE) {
src1 = gen_get_ccr ();
} else {
...
}
if (op == 0) {
tcg_gen_ori_i32 ...
} else {
tcg_gen_andi_i32 ...
}
gen_set_sr(s, dest, opsize == OS_BYTE);
return;
}
// existing code
r~
[Qemu-devel] [PATCH for-2.5 17/30] m68k: ori/andi/subi/addi/eori/cmpi can modify SR/CCR, Laurent Vivier, 2015/08/09
- Re: [Qemu-devel] [PATCH for-2.5 17/30] m68k: ori/andi/subi/addi/eori/cmpi can modify SR/CCR,
Richard Henderson <=
[Qemu-devel] [PATCH for-2.5 18/30] m68k: addq/subq can work with all the data sizes., Laurent Vivier, 2015/08/09
[Qemu-devel] [PATCH for-2.5 16/30] m68k: Add all access modes and data sizes to some 680x0 instructions, Laurent Vivier, 2015/08/09
[Qemu-devel] [PATCH for-2.5 20/30] m68k: add exg, Laurent Vivier, 2015/08/09