Re: [Qemu-devel] [PATCH RFC v1 01/29] target-arc: initial commit

[Richard Henderson]

On 09/08/2016 03:31 PM, Michael Rolnik wrote:
> +#define CPU_IMM(env)    ((env)->r[62])
> +#define CPU_PCL(env)    ((env)->r[63])

You don't need to represent these as actual registers.  These are better 
considered placeholder regnums to be filled in with actual values during 
translation.

> +struct CPUARCState {
> +    uint32_t        r[64];
> +
> +    struct {
> +        uint32_t    Lf;
> +        uint32_t    Zf;     /*  zero                    */
> +        uint32_t    Nf;     /*  negative                */
> +        uint32_t    Cf;     /*  carry                   */
> +        uint32_t    Vf;     /*  overflow                */
> +        uint32_t    Uf;
> +
> +        uint32_t    DEf;
> +        uint32_t    AEf;
> +        uint32_t    A2f;    /*  interrupt 1 is active   */
> +        uint32_t    A1f;    /*  interrupt 2 is active   */
> +        uint32_t    E2f;    /*  interrupt 1 mask        */
> +        uint32_t    E1f;    /*  interrupt 2 mask        */
> +        uint32_t    Hf;     /*  halt                    */
> +    } stat, stat_l1, stat_l2, stat_er;

There is no reason to represent each bit as a whole word, and even less to have 
four copies.

Only the current NZCV bits are relevant for expansion to a word, and even then 
you should consider not canonicalizing N, Z and V to a pure boolean value.

> +
> +    struct {
> +        uint32_t    S2;
> +        uint32_t    S1;
> +        uint32_t    CS;
> +    } macmod;

Does CS really need to be represented at all?  It appears to me to be a field 
that you write to that clears S1 and S2.

> +    switch (n) {
> +        case 0x00 ... 0x3f: {
> +            val = env->r[n];
> +            break;
> +        }

Please use the proper format for all switch statements.

    switch (n) {
    case 0x00 ... 0x3f:
        val = env->r[n];
        break;

> +
> +TCGv_env cpu_env;
> +
> +TCGv     cpu_gp;        /*  Global Pointer                      */
> +TCGv     cpu_fp;        /*  Frame Pointer                       */
> +TCGv     cpu_sp;        /*  Stack Pointer                       */
> +TCGv     cpu_ilink1;    /*  Level 1 interrupt link register     */
> +TCGv     cpu_ilink2;    /*  Level 2 interrupt link register     */
> +TCGv     cpu_blink;     /*  Branch link register                */
> +TCGv     cpu_mlo;       /*  Multiply low 32 bits, read only     */
> +TCGv     cpu_mmi;       /*  Multiply middle 32 bits, read only  */
> +TCGv     cpu_mhi;       /*  Multiply high 32 bits, read only    */

Why are these separate variables?  They overlap cpu_r[N].  If you want them as 
names for clarity in translation, #define seems good enough.

> +int arc_gen_INVALID(DisasCtxt *ctx)
> +{
> +    printf("invalid inst @:%08x\n", ctx->cpc);

No printf.  It's not useful, even temporarily.

> +    ctx.zero = tcg_const_local_i32(0);
> +    ctx.one = tcg_const_local_i32(1);
> +    ctx.msb32 = tcg_const_local_i32(0x80000000);
> +    ctx.msb16 = tcg_const_local_i32(0x00008000);
> +    ctx.smax16 = tcg_const_local_i32(0x7fffffff);
> +    ctx.smax32 = tcg_const_local_i32(0x00007fff);
> +    ctx.smax5 = tcg_const_local_i32(0x0000001f);
> +    ctx.smin5 = tcg_const_local_i32(0xffffffe1);

Why are you creating all of these?  Creating local temps containing immediates 
is a horrible waste.

> +        if (ctx.npc == env->lpe) {

You can't look at the contents of ENV during translation.

You'll need to implement this feature similar to how it's done for xtensa.  See 
helper_wsr_lbeg, helper_wsr_lend, and gen_check_loop_end.


r~