Re: [Qemu-devel] [PATCH v2 1/4] Add moxie target code

[Richard Henderson]

On 2013-02-24 05:11, Anthony Green wrote:
> +static TCGv cpu_pc;
> +static TCGv cpu_gregs[16];
> +static TCGv cpu_sregs[256];
...
> +    for (i = 0; i < 256; i++)
> +        cpu_sregs[i] = tcg_global_mem_new_i32(TCG_AREG0,
> +                                              offsetof(CPUMoxieState, 
> sregs[i]),
> +                                              sregnames[i]);

Still creating 256 registers for sregs.  With bogus names to boot.
And points out that you aren't adding the braces required by coding
style.  Please use ./scripts/checkpatch.pl.

> +        case 0x03: /* jsra */
> +            {
> +                /* Load the stack pointer into T0.  */
> +                TCGv t1 = tcg_temp_new_i32();
> +                TCGv t2 = tcg_temp_new_i32();
> +
> +                tcg_gen_movi_i32(t1, ctx->pc+6);
> +
> +                /* Make space for the static chain and return address.  */
> +                tcg_gen_subi_i32(t2, REG(1), 8);
> +                tcg_gen_mov_i32(REG(1), t2);
> +                tcg_gen_qemu_st32(t1, REG(1), ctx->memidx);
> +
> +                /* Push the current frame pointer.  */
> +                tcg_gen_subi_i32(t2, REG(1), 4);
> +                tcg_gen_mov_i32(REG(1), t2);
> +                tcg_gen_qemu_st32(REG(0), REG(1), ctx->memidx);

Surely you don't want to update REG(1) in the middle of the insn.
If one of those stores faults, then the insn can't be restarted.
I mentioned that last time.

> +        case 0x04: /* ret */
> +        case 0x06: /* push */
> +        case 0x07: /* pop */
> +        case 0x19: /* jsr */
> +        case 0x30: /* swi */

Likewise.

> +        case 0x26: /* and */
> +            {
> +                int a = (opcode >> 4) & 0xf;
> +                int b = opcode & 0xf;
> +                TCGv t1 = tcg_temp_new_i32();
> +                tcg_gen_and_i32(t1, REG(a), REG(b));
> +                tcg_gen_mov_i32(REG(a), t1);
> +                tcg_temp_free_i32(t1);
> +            }
> +            break;

Stopped cleaning up temporaries this far down?

> +        case 0x27: /* lshr */
> +        case 0x28: /* ashl */
> +        case 0x2b: /* or */
> +        case 0x2d: /* ashr */

Likewise.


r~