Patchwork [04/13] tcg: sync output arguments on liveness request

login
register
mail settings
Submitter Aurelien Jarno
Date Sept. 27, 2012, 5:15 p.m.
Message ID <1348766113-18373-5-git-send-email-aurelien@aurel32.net>
Download mbox | patch
Permalink /patch/187413/
State New
Headers show

Comments

Aurelien Jarno - Sept. 27, 2012, 5:15 p.m.
Synchronize an output argument when requested by the liveness analysis.
This is needed so that the temp can be declared dead later.

For that, add a new op_sync_args table in which each bit tells if the
corresponding output argument needs to be synchronized with the memory.
Pass it to the tcg_reg_alloc_* functions, and honor this bit. We need to
synchronize the argument before marking it as dead, and we have to make
sure all the infos about the temp are correctly filled.

At the same time change some types from unsigned int to uint16_t when
passing op_dead_args.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/tcg.c |   97 ++++++++++++++++++++++++++++++++++++++-----------------------
 tcg/tcg.h |    3 ++
 2 files changed, 64 insertions(+), 36 deletions(-)
Richard Henderson - Sept. 27, 2012, 6:39 p.m.
On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> Synchronize an output argument when requested by the liveness analysis.
> This is needed so that the temp can be declared dead later.
> 
> For that, add a new op_sync_args table in which each bit tells if the
> corresponding output argument needs to be synchronized with the memory.
> Pass it to the tcg_reg_alloc_* functions, and honor this bit. We need to
> synchronize the argument before marking it as dead, and we have to make
> sure all the infos about the temp are correctly filled.
> 
> At the same time change some types from unsigned int to uint16_t when
> passing op_dead_args.
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

If I understand the patch correctly, this patch alone is no change,
since no sync_args bits are ever set.

Which means that

> +        if (NEED_SYNC_ARG(i)) {
> +            tcg_reg_sync(s, reg);
> +        }
> +        if (IS_DEAD_ARG(i)) {
> +            temp_dead(s, args[i]);
> +        }

Might ought to be better written as

  if (IS_DEAD_ARG(i)) {
     ...
  } else if (NEED_SYNC_ARG(i)) {
     ...
  }

as temp_dead implies sync from patches 2 and 3.  This pattern is also
replicated 3-4 times.  Subroutine?


r~
Aurelien Jarno - Sept. 27, 2012, 8:05 p.m.
On Thu, Sep 27, 2012 at 11:39:06AM -0700, Richard Henderson wrote:
> On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> > Synchronize an output argument when requested by the liveness analysis.
> > This is needed so that the temp can be declared dead later.
> > 
> > For that, add a new op_sync_args table in which each bit tells if the
> > corresponding output argument needs to be synchronized with the memory.
> > Pass it to the tcg_reg_alloc_* functions, and honor this bit. We need to
> > synchronize the argument before marking it as dead, and we have to make
> > sure all the infos about the temp are correctly filled.
> > 
> > At the same time change some types from unsigned int to uint16_t when
> > passing op_dead_args.
> > 
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> If I understand the patch correctly, this patch alone is no change,
> since no sync_args bits are ever set.

They are set later in this series, this is a preparatory work.

> Which means that
> 
> > +        if (NEED_SYNC_ARG(i)) {
> > +            tcg_reg_sync(s, reg);
> > +        }
> > +        if (IS_DEAD_ARG(i)) {
> > +            temp_dead(s, args[i]);
> > +        }
> 
> Might ought to be better written as
> 
>   if (IS_DEAD_ARG(i)) {
>      ...
>   } else if (NEED_SYNC_ARG(i)) {
>      ...
>   }
> 
> as temp_dead implies sync from patches 2 and 3.  This pattern is also

No temp_dead doesn't imply sync. temp_dead is only marking the temp as
dead, it doesn' save it. 

> replicated 3-4 times.  Subroutine?
> 

Replicated in this patch, but only 2 are left at the end.
Richard Henderson - Sept. 27, 2012, 8:10 p.m.
On 09/27/2012 01:05 PM, Aurelien Jarno wrote:
> No temp_dead doesn't imply sync. temp_dead is only marking the temp as
> dead, it doesn' save it. 

Oh, duh.  How did I mis-read "dead" as "save"?


r~
Aurelien Jarno - Sept. 27, 2012, 8:25 p.m.
On Thu, Sep 27, 2012 at 01:10:28PM -0700, Richard Henderson wrote:
> On 09/27/2012 01:05 PM, Aurelien Jarno wrote:
> > No temp_dead doesn't imply sync. temp_dead is only marking the temp as
> > dead, it doesn' save it. 
> 
> Oh, duh.  How did I mis-read "dead" as "save"?
> 

Looks like ;)

Patch

diff --git a/tcg/tcg.c b/tcg/tcg.c
index fb2223f..b89ab04 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1221,13 +1221,15 @@  static void tcg_liveness_analysis(TCGContext *s)
     TCGArg *args;
     const TCGOpDef *def;
     uint8_t *dead_temps;
-    unsigned int dead_args;
+    uint16_t dead_args;
+    uint8_t sync_args;
     
     gen_opc_ptr++; /* skip end */
 
     nb_ops = gen_opc_ptr - gen_opc_buf;
 
     s->op_dead_args = tcg_malloc(nb_ops * sizeof(uint16_t));
+    s->op_sync_args = tcg_malloc(nb_ops * sizeof(uint8_t));
     
     dead_temps = tcg_malloc(s->nb_temps);
     memset(dead_temps, 1, s->nb_temps);
@@ -1264,6 +1266,7 @@  static void tcg_liveness_analysis(TCGContext *s)
 
                     /* output args are dead */
                     dead_args = 0;
+                    sync_args = 0;
                     for(i = 0; i < nb_oargs; i++) {
                         arg = args[i];
                         if (dead_temps[arg]) {
@@ -1288,6 +1291,7 @@  static void tcg_liveness_analysis(TCGContext *s)
                         }
                     }
                     s->op_dead_args[op_index] = dead_args;
+                    s->op_sync_args[op_index] = sync_args;
                 }
                 args--;
             }
@@ -1330,6 +1334,7 @@  static void tcg_liveness_analysis(TCGContext *s)
 
                 /* output args are dead */
                 dead_args = 0;
+                sync_args = 0;
                 for(i = 0; i < nb_oargs; i++) {
                     arg = args[i];
                     if (dead_temps[arg]) {
@@ -1355,6 +1360,7 @@  static void tcg_liveness_analysis(TCGContext *s)
                     dead_temps[arg] = 0;
                 }
                 s->op_dead_args[op_index] = dead_args;
+                s->op_sync_args[op_index] = sync_args;
             }
             break;
         }
@@ -1373,6 +1379,8 @@  static void tcg_liveness_analysis(TCGContext *s)
 
     s->op_dead_args = tcg_malloc(nb_ops * sizeof(uint16_t));
     memset(s->op_dead_args, 0, nb_ops * sizeof(uint16_t));
+    s->op_sync_args = tcg_malloc(nb_ops * sizeof(uint8_t));
+    memset(s->op_sync_args, 0, nb_ops * sizeof(uint8_t));
 }
 #endif
 
@@ -1615,8 +1623,10 @@  static void tcg_reg_alloc_bb_end(TCGContext *s, TCGRegSet allocated_regs)
 }
 
 #define IS_DEAD_ARG(n) ((dead_args >> (n)) & 1)
+#define NEED_SYNC_ARG(n) ((sync_args >> (n)) & 1)
 
-static void tcg_reg_alloc_movi(TCGContext *s, const TCGArg *args)
+static void tcg_reg_alloc_movi(TCGContext *s, const TCGArg *args,
+                               uint16_t dead_args, uint8_t sync_args)
 {
     TCGTemp *ots;
     tcg_target_ulong val;
@@ -1635,11 +1645,14 @@  static void tcg_reg_alloc_movi(TCGContext *s, const TCGArg *args)
         ots->val_type = TEMP_VAL_CONST;
         ots->val = val;
     }
+    if (NEED_SYNC_ARG(0)) {
+        temp_sync(s, args[0], s->reserved_regs);
+    }
 }
 
 static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
-                              const TCGArg *args,
-                              unsigned int dead_args)
+                              const TCGArg *args, uint16_t dead_args,
+                              uint8_t sync_args)
 {
     TCGTemp *ts, *ots;
     int reg;
@@ -1684,6 +1697,9 @@  static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
                 s->reg_to_temp[ots->reg] = -1;
             ots->val_type = TEMP_VAL_CONST;
             ots->val = ts->val;
+            if (NEED_SYNC_ARG(0)) {
+                temp_sync(s, args[0], s->reserved_regs);
+            }
             return;
         }
     } else {
@@ -1693,12 +1709,16 @@  static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
     ots->reg = reg;
     ots->val_type = TEMP_VAL_REG;
     ots->mem_coherent = 0;
+
+    if (NEED_SYNC_ARG(0)) {
+        tcg_reg_sync(s, reg);
+    }
 }
 
 static void tcg_reg_alloc_op(TCGContext *s, 
                              const TCGOpDef *def, TCGOpcode opc,
-                             const TCGArg *args,
-                             unsigned int dead_args)
+                             const TCGArg *args, uint16_t dead_args,
+                             uint8_t sync_args)
 {
     TCGRegSet allocated_regs;
     int i, k, nb_iargs, nb_oargs, reg;
@@ -1824,19 +1844,15 @@  static void tcg_reg_alloc_op(TCGContext *s,
             tcg_regset_set_reg(allocated_regs, reg);
             /* if a fixed register is used, then a move will be done afterwards */
             if (!ts->fixed_reg) {
-                if (IS_DEAD_ARG(i)) {
-                    temp_dead(s, args[i]);
-                } else {
-                    if (ts->val_type == TEMP_VAL_REG) {
-                        s->reg_to_temp[ts->reg] = -1;
-                    }
-                    ts->val_type = TEMP_VAL_REG;
-                    ts->reg = reg;
-                    /* temp value is modified, so the value kept in memory is
-                       potentially not the same */
-                    ts->mem_coherent = 0;
-                    s->reg_to_temp[reg] = arg;
-               }
+                if (ts->val_type == TEMP_VAL_REG) {
+                    s->reg_to_temp[ts->reg] = -1;
+                }
+                ts->val_type = TEMP_VAL_REG;
+                ts->reg = reg;
+                /* temp value is modified, so the value kept in memory is
+                   potentially not the same */
+                ts->mem_coherent = 0;
+                s->reg_to_temp[reg] = arg;
             }
         oarg_end:
             new_args[i] = reg;
@@ -1853,6 +1869,12 @@  static void tcg_reg_alloc_op(TCGContext *s,
         if (ts->fixed_reg && ts->reg != reg) {
             tcg_out_mov(s, ts->type, ts->reg, reg);
         }
+        if (NEED_SYNC_ARG(i)) {
+            tcg_reg_sync(s, reg);
+        }
+        if (IS_DEAD_ARG(i)) {
+            temp_dead(s, args[i]);
+        }
     }
 }
 
@@ -1864,7 +1886,7 @@  static void tcg_reg_alloc_op(TCGContext *s,
 
 static int tcg_reg_alloc_call(TCGContext *s, const TCGOpDef *def,
                               TCGOpcode opc, const TCGArg *args,
-                              unsigned int dead_args)
+                              uint16_t dead_args, uint8_t sync_args)
 {
     int nb_iargs, nb_oargs, flags, nb_regs, i, reg, nb_params;
     TCGArg arg, func_arg;
@@ -2019,16 +2041,18 @@  static int tcg_reg_alloc_call(TCGContext *s, const TCGOpDef *def,
                 tcg_out_mov(s, ts->type, ts->reg, reg);
             }
         } else {
+            if (ts->val_type == TEMP_VAL_REG) {
+                s->reg_to_temp[ts->reg] = -1;
+            }
+            ts->val_type = TEMP_VAL_REG;
+            ts->reg = reg;
+            ts->mem_coherent = 0;
+            s->reg_to_temp[reg] = arg;
+            if (NEED_SYNC_ARG(i)) {
+                tcg_reg_sync(s, reg);
+            }
             if (IS_DEAD_ARG(i)) {
                 temp_dead(s, args[i]);
-            } else {
-                if (ts->val_type == TEMP_VAL_REG) {
-                    s->reg_to_temp[ts->reg] = -1;
-                }
-                ts->val_type = TEMP_VAL_REG;
-                ts->reg = reg;
-                ts->mem_coherent = 0;
-                s->reg_to_temp[reg] = arg;
             }
         }
     }
@@ -2059,7 +2083,6 @@  static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
     TCGOpcode opc;
     int op_index;
     const TCGOpDef *def;
-    unsigned int dead_args;
     const TCGArg *args;
 
 #ifdef DEBUG_DISAS
@@ -2120,12 +2143,13 @@  static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
         switch(opc) {
         case INDEX_op_mov_i32:
         case INDEX_op_mov_i64:
-            dead_args = s->op_dead_args[op_index];
-            tcg_reg_alloc_mov(s, def, args, dead_args);
+            tcg_reg_alloc_mov(s, def, args, s->op_dead_args[op_index],
+                              s->op_sync_args[op_index]);
             break;
         case INDEX_op_movi_i32:
         case INDEX_op_movi_i64:
-            tcg_reg_alloc_movi(s, args);
+            tcg_reg_alloc_movi(s, args, s->op_dead_args[op_index],
+                               s->op_sync_args[op_index]);
             break;
         case INDEX_op_debug_insn_start:
             /* debug instruction */
@@ -2146,8 +2170,9 @@  static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
             tcg_out_label(s, args[0], s->code_ptr);
             break;
         case INDEX_op_call:
-            dead_args = s->op_dead_args[op_index];
-            args += tcg_reg_alloc_call(s, def, opc, args, dead_args);
+            args += tcg_reg_alloc_call(s, def, opc, args,
+                                       s->op_dead_args[op_index],
+                                       s->op_sync_args[op_index]);
             goto next;
         case INDEX_op_end:
             goto the_end;
@@ -2159,8 +2184,8 @@  static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
             /* Note: in order to speed up the code, it would be much
                faster to have specialized register allocator functions for
                some common argument patterns */
-            dead_args = s->op_dead_args[op_index];
-            tcg_reg_alloc_op(s, def, opc, args, dead_args);
+            tcg_reg_alloc_op(s, def, opc, args, s->op_dead_args[op_index],
+                             s->op_sync_args[op_index]);
             break;
         }
         args += def->nb_args;
diff --git a/tcg/tcg.h b/tcg/tcg.h
index af7464a..6875bc1 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -351,6 +351,9 @@  struct TCGContext {
     /* liveness analysis */
     uint16_t *op_dead_args; /* for each operation, each bit tells if the
                                corresponding argument is dead */
+    uint8_t *op_sync_args;  /* for each operation, each bit tells if the
+                               corresponding output argument needs to be
+                               sync to memory. */
     
     /* tells in which temporary a given register is. It does not take
        into account fixed registers */