qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Richard Henderson
Subject: Re: [Qemu-devel] [PATCH RFC v1 01/29] target-arc: initial commit
Date: Tue, 20 Sep 2016 16:31:03 -0700
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0

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~



reply via email to

[Prev in Thread] Current Thread [Next in Thread]