Patchwork [07/13] tcg: rewrite tcg_reg_alloc_mov()

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

Comments

Aurelien Jarno - Sept. 27, 2012, 5:15 p.m.
Now that the liveness analysis provides more information, rewrite
tcg_reg_alloc_mov(). This changes the behaviour about propagating
constants and memory accesses. We now take the assumption that once
a value is loaded into a register (from memory or from a constant),
it's better to keep it there than to reload it later. This assumption
is now always almost correct given that we are now sure the
corresponding temp is going to be used later (otherwise it would have
been synchronized and marked as dead already). The assumption is wrong
if one of the op after clobbers some registers including the one
of the holding the temp (this can be avoided by allocating clobbered
registers last, which is what most TCG target do), or in case of lack
of available register.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/tcg.c |  105 ++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 63 insertions(+), 42 deletions(-)
Richard Henderson - Sept. 27, 2012, 7:09 p.m.
On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> +    /* We have to load the value in a register for moving it to another
> +       or for saving it. We assume it's better to keep it there than to
> +       reload it later. */
> +    if ((NEED_SYNC_ARG(0) && ts->val_type != TEMP_VAL_REG)
> +        || ts->val_type == TEMP_VAL_MEM) {
> +        ts->reg = tcg_reg_alloc(s, arg_ct->u.regs, allocated_regs);
> +        if (ts->val_type == TEMP_VAL_MEM) {
> +            tcg_out_ld(s, ts->type, ts->reg, ts->mem_reg, ts->mem_offset);
> +        } else if (ts->val_type == TEMP_VAL_CONST) {
> +            tcg_out_movi(s, ts->type, ts->reg, ts->val);
> +        }
> +        s->reg_to_temp[ts->reg] = args[1];
> +        ts->val_type = TEMP_VAL_REG;
> +        ts->mem_coherent = 1;
> +    }

I don't understand this block.  In particular, the ts->mem_coherent = 1
in the TEMP_VAL_CONST case looks wrong.

Why are you handling NEED_SYNC_ARG before the move, rather than after?

> +    if (IS_DEAD_ARG(0) && !ots->fixed_reg) {
> +        /* mov to a non-saved dead register makes no sense (even with
> +           liveness analysis disabled). */
> +        assert(NEED_SYNC_ARG(0));
> +        /* The code above should have moved the temp to a register. */
> +        assert(ts->val_type == TEMP_VAL_REG);
> +        if (!ots->mem_allocated) {
> +            temp_allocate_frame(s, args[0]);
>          }
> +        if (ts->val_type == TEMP_VAL_REG) {
> +            tcg_out_st(s, ots->type, ts->reg, ots->mem_reg, ots->mem_offset);
> +            if (IS_DEAD_ARG(1)) {
> +                temp_dead(s, args[1]);
> +            }
>          }
> +        temp_dead(s, args[0]);

Isn't this store going to happen via temp_dead -> temp_sync -> tcg_reg_sync?

>      } else if (ts->val_type == TEMP_VAL_CONST) {
>          if (ots->fixed_reg) {
> +            tcg_out_movi(s, ots->type, ots->reg, ts->val);
> +            s->reg_to_temp[ots->reg] = args[0];
>          } else {
>              /* propagate constant */
> +            if (ots->val_type == TEMP_VAL_REG) {
>                  s->reg_to_temp[ots->reg] = -1;
> +            }
>              ots->val_type = TEMP_VAL_CONST;
>              ots->val = ts->val;
>          }

How much of the first block above is redundant with this?
Especially given that I think there's a missing sync.

>      } else {
> +        /* The code above should have moved the temp to a register. */
> +        assert(ts->val_type == TEMP_VAL_REG);
> +        if (IS_DEAD_ARG(1) && !ts->fixed_reg && !ots->fixed_reg) {
> +                /* the mov can be suppressed */
> +                if (ots->val_type == TEMP_VAL_REG) {
> +                    s->reg_to_temp[ots->reg] = -1;
> +                }
> +                ots->reg = ts->reg;
> +                temp_dead(s, args[1]);
> +        } else {
> +            if (ots->val_type != TEMP_VAL_REG) {
> +                /* When allocating a new register, make sure to not spill the
> +                   input one. */
> +                tcg_regset_set_reg(allocated_regs, ts->reg);
> +                ots->reg = tcg_reg_alloc(s, oarg_ct->u.regs, allocated_regs);
> +            }
> +            tcg_out_mov(s, ots->type, ots->reg, ts->reg);
> +        }
> +        ots->val_type = TEMP_VAL_REG;
> +        ots->mem_coherent = 0;
> +        s->reg_to_temp[ots->reg] = args[0];
> +        if (NEED_SYNC_ARG(0)) {
> +            tcg_reg_sync(s, ots->reg);
> +        }

... as we do here.


r~
Aurelien Jarno - Sept. 27, 2012, 8:17 p.m.
On Thu, Sep 27, 2012 at 12:09:35PM -0700, Richard Henderson wrote:
> On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> > +    /* We have to load the value in a register for moving it to another
> > +       or for saving it. We assume it's better to keep it there than to
> > +       reload it later. */
> > +    if ((NEED_SYNC_ARG(0) && ts->val_type != TEMP_VAL_REG)
> > +        || ts->val_type == TEMP_VAL_MEM) {
> > +        ts->reg = tcg_reg_alloc(s, arg_ct->u.regs, allocated_regs);
> > +        if (ts->val_type == TEMP_VAL_MEM) {
> > +            tcg_out_ld(s, ts->type, ts->reg, ts->mem_reg, ts->mem_offset);
> > +        } else if (ts->val_type == TEMP_VAL_CONST) {
> > +            tcg_out_movi(s, ts->type, ts->reg, ts->val);
> > +        }
> > +        s->reg_to_temp[ts->reg] = args[1];
> > +        ts->val_type = TEMP_VAL_REG;
> > +        ts->mem_coherent = 1;
> > +    }
> 
> I don't understand this block.  In particular, the ts->mem_coherent = 1
> in the TEMP_VAL_CONST case looks wrong.

Indeed the ts->mem_coherent = 1 is wrong in the TEMP_VAL_CONST.

> Why are you handling NEED_SYNC_ARG before the move, rather than after?

Because the move is likely to be eliminated by the code below,
especially if the temp is dead in addition.

> > +    if (IS_DEAD_ARG(0) && !ots->fixed_reg) {
> > +        /* mov to a non-saved dead register makes no sense (even with
> > +           liveness analysis disabled). */
> > +        assert(NEED_SYNC_ARG(0));
> > +        /* The code above should have moved the temp to a register. */
> > +        assert(ts->val_type == TEMP_VAL_REG);
> > +        if (!ots->mem_allocated) {
> > +            temp_allocate_frame(s, args[0]);
> >          }
> > +        if (ts->val_type == TEMP_VAL_REG) {
> > +            tcg_out_st(s, ots->type, ts->reg, ots->mem_reg, ots->mem_offset);
> > +            if (IS_DEAD_ARG(1)) {
> > +                temp_dead(s, args[1]);
> > +            }
> >          }
> > +        temp_dead(s, args[0]);
> 
> Isn't this store going to happen via temp_dead -> temp_sync -> tcg_reg_sync?

temp_dead only mark the temp as dead, it doesn't save it.

> >      } else if (ts->val_type == TEMP_VAL_CONST) {
> >          if (ots->fixed_reg) {
> > +            tcg_out_movi(s, ots->type, ots->reg, ts->val);
> > +            s->reg_to_temp[ots->reg] = args[0];
> >          } else {
> >              /* propagate constant */
> > +            if (ots->val_type == TEMP_VAL_REG) {
> >                  s->reg_to_temp[ots->reg] = -1;
> > +            }
> >              ots->val_type = TEMP_VAL_CONST;
> >              ots->val = ts->val;
> >          }
> 
> How much of the first block above is redundant with this?
> Especially given that I think there's a missing sync.

The goal of to first block is to move the value to a register in case a
sync is needed, and the sync is done at this moment. The ots->fixed_reg
can indeed by merged into the first block, but the rest has to stay there.

> >      } else {
> > +        /* The code above should have moved the temp to a register. */
> > +        assert(ts->val_type == TEMP_VAL_REG);
> > +        if (IS_DEAD_ARG(1) && !ts->fixed_reg && !ots->fixed_reg) {
> > +                /* the mov can be suppressed */
> > +                if (ots->val_type == TEMP_VAL_REG) {
> > +                    s->reg_to_temp[ots->reg] = -1;
> > +                }
> > +                ots->reg = ts->reg;
> > +                temp_dead(s, args[1]);
> > +        } else {
> > +            if (ots->val_type != TEMP_VAL_REG) {
> > +                /* When allocating a new register, make sure to not spill the
> > +                   input one. */
> > +                tcg_regset_set_reg(allocated_regs, ts->reg);
> > +                ots->reg = tcg_reg_alloc(s, oarg_ct->u.regs, allocated_regs);
> > +            }
> > +            tcg_out_mov(s, ots->type, ots->reg, ts->reg);
> > +        }
> > +        ots->val_type = TEMP_VAL_REG;
> > +        ots->mem_coherent = 0;
> > +        s->reg_to_temp[ots->reg] = args[0];
> > +        if (NEED_SYNC_ARG(0)) {
> > +            tcg_reg_sync(s, ots->reg);
> > +        }
> 
> ... as we do here.

The sync is done here because it has not been done in the first block.
It's different than in the ts->val_type == TEMP_VAL_CONST
Richard Henderson - Sept. 27, 2012, 10:18 p.m.
On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> +    /* We have to load the value in a register for moving it to another
> +       or for saving it. We assume it's better to keep it there than to
> +       reload it later. */
> +    if ((NEED_SYNC_ARG(0) && ts->val_type != TEMP_VAL_REG)
> +        || ts->val_type == TEMP_VAL_MEM) {
> +        ts->reg = tcg_reg_alloc(s, arg_ct->u.regs, allocated_regs);
> +        if (ts->val_type == TEMP_VAL_MEM) {
> +            tcg_out_ld(s, ts->type, ts->reg, ts->mem_reg, ts->mem_offset);
> +        } else if (ts->val_type == TEMP_VAL_CONST) {
> +            tcg_out_movi(s, ts->type, ts->reg, ts->val);
> +        }
> +        s->reg_to_temp[ts->reg] = args[1];
> +        ts->val_type = TEMP_VAL_REG;
> +        ts->mem_coherent = 1;
> +    }

Ok, I believe I understand what's going on now.  Nothing like trying to
rewrite the function yourself to figure out why you've done things this way.

The only thing I'd change for clarity is that first condition:

    /* If the source value is not in a register, and we're going to be
       forced to have it in a register in order to perform the copy,
       then copy the SOURCE value into its own register first.  That way
       we don't have to reload SOURCE the next time it is used.

       Note that in the CONST + SYNC case, we must for the moment have
       the value in a register because we have no direct access to a
       store constant primitive.  */

    if ((ts->val_type == TEMP_VAL_CONST && NEED_SYNC_ARG(0))
        || ts->val_type == TEMP_VAL_MEM) {



r~

Patch

diff --git a/tcg/tcg.c b/tcg/tcg.c
index bfe6411..5fb4901 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1667,64 +1667,85 @@  static void tcg_reg_alloc_mov(TCGContext *s, const TCGOpDef *def,
                               const TCGArg *args, uint16_t dead_args,
                               uint8_t sync_args)
 {
+    TCGRegSet allocated_regs;
     TCGTemp *ts, *ots;
-    int reg;
-    const TCGArgConstraint *arg_ct;
+    const TCGArgConstraint *arg_ct, *oarg_ct;
 
+    tcg_regset_set(allocated_regs, s->reserved_regs);
     ots = &s->temps[args[0]];
     ts = &s->temps[args[1]];
-    arg_ct = &def->args_ct[0];
+    oarg_ct = &def->args_ct[0];
+    arg_ct = &def->args_ct[1];
+
+    /* We have to load the value in a register for moving it to another
+       or for saving it. We assume it's better to keep it there than to
+       reload it later. */
+    if ((NEED_SYNC_ARG(0) && ts->val_type != TEMP_VAL_REG)
+        || ts->val_type == TEMP_VAL_MEM) {
+        ts->reg = tcg_reg_alloc(s, arg_ct->u.regs, allocated_regs);
+        if (ts->val_type == TEMP_VAL_MEM) {
+            tcg_out_ld(s, ts->type, ts->reg, ts->mem_reg, ts->mem_offset);
+        } else if (ts->val_type == TEMP_VAL_CONST) {
+            tcg_out_movi(s, ts->type, ts->reg, ts->val);
+        }
+        s->reg_to_temp[ts->reg] = args[1];
+        ts->val_type = TEMP_VAL_REG;
+        ts->mem_coherent = 1;
+    }
 
-    /* XXX: always mark arg dead if IS_DEAD_ARG(1) */
-    if (ts->val_type == TEMP_VAL_REG) {
-        if (IS_DEAD_ARG(1) && !ts->fixed_reg && !ots->fixed_reg) {
-            /* the mov can be suppressed */
-            if (ots->val_type == TEMP_VAL_REG)
-                s->reg_to_temp[ots->reg] = -1;
-            reg = ts->reg;
-            temp_dead(s, args[1]);
-        } else {
-            if (ots->val_type == TEMP_VAL_REG) {
-                reg = ots->reg;
-            } else {
-                reg = tcg_reg_alloc(s, arg_ct->u.regs, s->reserved_regs);
-            }
-            if (ts->reg != reg) {
-                tcg_out_mov(s, ots->type, reg, ts->reg);
-            }
+    if (IS_DEAD_ARG(0) && !ots->fixed_reg) {
+        /* mov to a non-saved dead register makes no sense (even with
+           liveness analysis disabled). */
+        assert(NEED_SYNC_ARG(0));
+        /* The code above should have moved the temp to a register. */
+        assert(ts->val_type == TEMP_VAL_REG);
+        if (!ots->mem_allocated) {
+            temp_allocate_frame(s, args[0]);
         }
-    } else if (ts->val_type == TEMP_VAL_MEM) {
-        if (ots->val_type == TEMP_VAL_REG) {
-            reg = ots->reg;
-        } else {
-            reg = tcg_reg_alloc(s, arg_ct->u.regs, s->reserved_regs);
+        if (ts->val_type == TEMP_VAL_REG) {
+            tcg_out_st(s, ots->type, ts->reg, ots->mem_reg, ots->mem_offset);
+            if (IS_DEAD_ARG(1)) {
+                temp_dead(s, args[1]);
+            }
         }
-        tcg_out_ld(s, ts->type, reg, ts->mem_reg, ts->mem_offset);
+        temp_dead(s, args[0]);
     } else if (ts->val_type == TEMP_VAL_CONST) {
         if (ots->fixed_reg) {
-            reg = ots->reg;
-            tcg_out_movi(s, ots->type, reg, ts->val);
+            tcg_out_movi(s, ots->type, ots->reg, ts->val);
+            s->reg_to_temp[ots->reg] = args[0];
         } else {
             /* propagate constant */
-            if (ots->val_type == TEMP_VAL_REG)
+            if (ots->val_type == TEMP_VAL_REG) {
                 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 {
-        tcg_abort();
-    }
-    s->reg_to_temp[reg] = args[0];
-    ots->reg = reg;
-    ots->val_type = TEMP_VAL_REG;
-    ots->mem_coherent = 0;
-
-    if (NEED_SYNC_ARG(0)) {
-        tcg_reg_sync(s, reg);
+        /* The code above should have moved the temp to a register. */
+        assert(ts->val_type == TEMP_VAL_REG);
+        if (IS_DEAD_ARG(1) && !ts->fixed_reg && !ots->fixed_reg) {
+                /* the mov can be suppressed */
+                if (ots->val_type == TEMP_VAL_REG) {
+                    s->reg_to_temp[ots->reg] = -1;
+                }
+                ots->reg = ts->reg;
+                temp_dead(s, args[1]);
+        } else {
+            if (ots->val_type != TEMP_VAL_REG) {
+                /* When allocating a new register, make sure to not spill the
+                   input one. */
+                tcg_regset_set_reg(allocated_regs, ts->reg);
+                ots->reg = tcg_reg_alloc(s, oarg_ct->u.regs, allocated_regs);
+            }
+            tcg_out_mov(s, ots->type, ots->reg, ts->reg);
+        }
+        ots->val_type = TEMP_VAL_REG;
+        ots->mem_coherent = 0;
+        s->reg_to_temp[ots->reg] = args[0];
+        if (NEED_SYNC_ARG(0)) {
+            tcg_reg_sync(s, ots->reg);
+        }
     }
 }