[06/16] tcg: Add temp_global bit to TCGTemp
diff mbox

Message ID 20170621024831.26019-7-rth@twiddle.net
State New
Headers show

Commit Message

Richard Henderson June 21, 2017, 2:48 a.m. UTC
This avoids needing to test the index of a temp against nb_globals.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/optimize.c | 15 ++++++++-------
 tcg/tcg.c      | 11 ++++++++---
 tcg/tcg.h      | 12 ++++++++----
 3 files changed, 24 insertions(+), 14 deletions(-)

Comments

Alex Bennée June 27, 2017, 8:39 a.m. UTC | #1
Richard Henderson <rth@twiddle.net> writes:

> This avoids needing to test the index of a temp against nb_globals.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/optimize.c | 15 ++++++++-------
>  tcg/tcg.c      | 11 ++++++++---
>  tcg/tcg.h      | 12 ++++++++----
>  3 files changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/tcg/optimize.c b/tcg/optimize.c
> index d8c3a7e..55f9e83 100644
> --- a/tcg/optimize.c
> +++ b/tcg/optimize.c
> @@ -116,25 +116,26 @@ static TCGOpcode op_to_movi(TCGOpcode op)
>      }
>  }
>
> -static TCGArg find_better_copy(TCGContext *s, TCGArg temp)
> +static TCGArg find_better_copy(TCGContext *s, TCGArg arg)
>  {
> +    TCGTemp *ts = arg_temp(arg);
>      TCGArg i;
>
>      /* If this is already a global, we can't do better. */
> -    if (temp < s->nb_globals) {
> -        return temp;
> +    if (ts->temp_global) {
> +        return arg;
>      }
>
>      /* Search for a global first. */
> -    for (i = temps[temp].next_copy ; i != temp ; i = temps[i].next_copy) {
> +    for (i = temps[arg].next_copy ; i != arg; i = temps[i].next_copy) {
>          if (i < s->nb_globals) {
>              return i;
>          }
>      }
>
>      /* If it is a temp, search for a temp local. */
> -    if (!arg_temp(temp)->temp_local) {
> -        for (i = temps[temp].next_copy ; i != temp ; i = temps[i].next_copy) {
> +    if (!ts->temp_local) {
> +        for (i = temps[arg].next_copy ; i != arg; i = temps[i].next_copy) {
>              if (s->temps[i].temp_local) {
>                  return i;
>              }
> @@ -142,7 +143,7 @@ static TCGArg find_better_copy(TCGContext *s, TCGArg temp)
>      }
>
>      /* Failure to find a better representation, return the same temp. */
> -    return temp;
> +    return arg;
>  }
>
>  static bool temps_are_copies(TCGArg arg1, TCGArg arg2)
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 068ac51..0bb88b1 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -489,9 +489,14 @@ static inline TCGTemp *tcg_temp_alloc(TCGContext *s)
>
>  static inline TCGTemp *tcg_global_alloc(TCGContext *s)
>  {
> +    TCGTemp *ts;
> +
>      tcg_debug_assert(s->nb_globals == s->nb_temps);
>      s->nb_globals++;
> -    return tcg_temp_alloc(s);
> +    ts = tcg_temp_alloc(s);
> +    ts->temp_global = 1;
> +
> +    return ts;
>  }
>
>  static int tcg_global_reg_new_internal(TCGContext *s, TCGType type,
> @@ -967,7 +972,7 @@ static char *tcg_get_arg_str_ptr(TCGContext *s, char *buf, int buf_size,
>  {
>      int idx = temp_idx(s, ts);
>
> -    if (idx < s->nb_globals) {
> +    if (ts->temp_global) {
>          pstrcpy(buf, buf_size, ts->name);
>      } else if (ts->temp_local) {
>          snprintf(buf, buf_size, "loc%d", idx - s->nb_globals);
> @@ -1905,7 +1910,7 @@ static void temp_free_or_dead(TCGContext *s, TCGTemp *ts, int free_or_dead)
>      }
>      ts->val_type = (free_or_dead < 0
>                      || ts->temp_local
> -                    || temp_idx(s, ts) < s->nb_globals
> +                    || ts->temp_global
>                      ? TEMP_VAL_MEM : TEMP_VAL_DEAD);
>  }
>
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 70d9fda..3b35344 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -586,10 +586,14 @@ typedef struct TCGTemp {
>      unsigned int indirect_base:1;
>      unsigned int mem_coherent:1;
>      unsigned int mem_allocated:1;
> -    unsigned int temp_local:1; /* If true, the temp is saved across
> -                                  basic blocks. Otherwise, it is not
> -                                  preserved across basic blocks. */
> -    unsigned int temp_allocated:1; /* never used for code gen */
> +    /* If true, the temp is saved across both basic blocks and
> +       translation blocks.  */
> +    unsigned int temp_global:1;
> +    /* If true, the temp is saved across basic blocks but dead
> +       at the end of translation blocks.  If false, the temp is
> +       dead at the end of basic blocks.  */
> +    unsigned int temp_local:1;
> +    unsigned int temp_allocated:1;

This is where my knowledge of the TCG internals gets slightly confused.
As far as I'm aware all our TranslationBlocks are Basic Blocks - they
don't have any branches until the end of the block. What is the
distinction here?

Is a temp_global truly global? I thought the guest state was fully
rectified by the time we leave the basic block.

>
>      tcg_target_long val;
>      struct TCGTemp *mem_base;


--
Alex Bennée
Richard Henderson June 27, 2017, 4:17 p.m. UTC | #2
On 06/27/2017 01:39 AM, Alex Bennée wrote:
>> +    /* If true, the temp is saved across both basic blocks and
>> +       translation blocks.  */
>> +    unsigned int temp_global:1;
>> +    /* If true, the temp is saved across basic blocks but dead
>> +       at the end of translation blocks.  If false, the temp is
>> +       dead at the end of basic blocks.  */
>> +    unsigned int temp_local:1;
>> +    unsigned int temp_allocated:1;
> 
> This is where my knowledge of the TCG internals gets slightly confused.
> As far as I'm aware all our TranslationBlocks are Basic Blocks - they
> don't have any branches until the end of the block. What is the
> distinction here?
> 
> Is a temp_global truly global? I thought the guest state was fully
> rectified by the time we leave the basic block.

TranslationBlocks are not basic blocks.  They normally stop at branches in the 
target instruction stream, but they certainly may have many branches in the tcg 
opcode stream (brcond and the like).  Consider, for instance, our 
implementation of arm32's conditional instructions.

Beyond that, I agree the language is confusing.

A temp_global is created by tcg_global_mem_new_*, generally represents a cpu 
register, and is synced back to a slot in ENV.

A temp_local is created by tcg_temp_local_new_*, and is synced to a slot in the 
local stack frame.

Something without either is simply declared dead at the end of a basic block, 
and is a source of confusion to those writing new front-ends.

Anyway, we already have all of these concepts.  The change is that before the 
patch the only way to tell a temp_global is to compare the index against 
tcg_ctx.nb_global.


r~
Alex Bennée June 28, 2017, 8:52 a.m. UTC | #3
Richard Henderson <rth@twiddle.net> writes:

> On 06/27/2017 01:39 AM, Alex Bennée wrote:
>>> +    /* If true, the temp is saved across both basic blocks and
>>> +       translation blocks.  */
>>> +    unsigned int temp_global:1;
>>> +    /* If true, the temp is saved across basic blocks but dead
>>> +       at the end of translation blocks.  If false, the temp is
>>> +       dead at the end of basic blocks.  */
>>> +    unsigned int temp_local:1;
>>> +    unsigned int temp_allocated:1;
>>
>> This is where my knowledge of the TCG internals gets slightly confused.
>> As far as I'm aware all our TranslationBlocks are Basic Blocks - they
>> don't have any branches until the end of the block. What is the
>> distinction here?
>>
>> Is a temp_global truly global? I thought the guest state was fully
>> rectified by the time we leave the basic block.
>
> TranslationBlocks are not basic blocks.  They normally stop at
> branches in the target instruction stream, but they certainly may have
> many branches in the tcg opcode stream (brcond and the like).
> Consider, for instance, our implementation of arm32's conditional
> instructions.

Right. Re-reading the tcg/README it does make the distinction but the
term "basic block" is overloaded depending on when talking about the
guest instructions or the generated code.

>
> Beyond that, I agree the language is confusing.
>
> A temp_global is created by tcg_global_mem_new_*, generally represents
> a cpu register, and is synced back to a slot in ENV.
>
> A temp_local is created by tcg_temp_local_new_*, and is synced to a
> slot in the local stack frame.

The language from the README:

  A TCG "temporary" is a variable only live in a basic
  block. Temporaries are allocated explicitly in each function.

  A TCG "local temporary" is a variable only live in a function. Local
  temporaries are allocated explicitly in each function.

I must admit I hadn't quite understood the distinction. In the ARM code
the only place where tcg_temp_local_new() over tcg_temp_new() is used is
in the ld/strex code. I guess because you need to preserve its value
over a potential TCG branch?

I guess in translate-a64/gen_store_exclusive() the key lines are:

  TCGv_i64 addr = tcg_temp_local_new_i64();

  /* Copy input into a local temp so it is not trashed when the
   * basic block ends at the branch insn.
   */
  tcg_gen_mov_i64(addr, inaddr);
  tcg_gen_brcond_i64(TCG_COND_NE, addr, cpu_exclusive_addr, fail_label);

> Something without either is simply declared dead at the end of a basic
> block, and is a source of confusion to those writing new front-ends.

I'll see if I can come up with some improved wording to help new
developers in the future.

> Anyway, we already have all of these concepts.  The change is that
> before the patch the only way to tell a temp_global is to compare the
> index against tcg_ctx.nb_global.

Indeed. As I have previously really only been a consumer of the TCG API
I'm taking the opportunity to learn more about the internals as the
vector work is likely to touch it ;-)

>
>
> r~


--
Alex Bennée

Patch
diff mbox

diff --git a/tcg/optimize.c b/tcg/optimize.c
index d8c3a7e..55f9e83 100644
--- a/tcg/optimize.c
+++ b/tcg/optimize.c
@@ -116,25 +116,26 @@  static TCGOpcode op_to_movi(TCGOpcode op)
     }
 }
 
-static TCGArg find_better_copy(TCGContext *s, TCGArg temp)
+static TCGArg find_better_copy(TCGContext *s, TCGArg arg)
 {
+    TCGTemp *ts = arg_temp(arg);
     TCGArg i;
 
     /* If this is already a global, we can't do better. */
-    if (temp < s->nb_globals) {
-        return temp;
+    if (ts->temp_global) {
+        return arg;
     }
 
     /* Search for a global first. */
-    for (i = temps[temp].next_copy ; i != temp ; i = temps[i].next_copy) {
+    for (i = temps[arg].next_copy ; i != arg; i = temps[i].next_copy) {
         if (i < s->nb_globals) {
             return i;
         }
     }
 
     /* If it is a temp, search for a temp local. */
-    if (!arg_temp(temp)->temp_local) {
-        for (i = temps[temp].next_copy ; i != temp ; i = temps[i].next_copy) {
+    if (!ts->temp_local) {
+        for (i = temps[arg].next_copy ; i != arg; i = temps[i].next_copy) {
             if (s->temps[i].temp_local) {
                 return i;
             }
@@ -142,7 +143,7 @@  static TCGArg find_better_copy(TCGContext *s, TCGArg temp)
     }
 
     /* Failure to find a better representation, return the same temp. */
-    return temp;
+    return arg;
 }
 
 static bool temps_are_copies(TCGArg arg1, TCGArg arg2)
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 068ac51..0bb88b1 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -489,9 +489,14 @@  static inline TCGTemp *tcg_temp_alloc(TCGContext *s)
 
 static inline TCGTemp *tcg_global_alloc(TCGContext *s)
 {
+    TCGTemp *ts;
+
     tcg_debug_assert(s->nb_globals == s->nb_temps);
     s->nb_globals++;
-    return tcg_temp_alloc(s);
+    ts = tcg_temp_alloc(s);
+    ts->temp_global = 1;
+
+    return ts;
 }
 
 static int tcg_global_reg_new_internal(TCGContext *s, TCGType type,
@@ -967,7 +972,7 @@  static char *tcg_get_arg_str_ptr(TCGContext *s, char *buf, int buf_size,
 {
     int idx = temp_idx(s, ts);
 
-    if (idx < s->nb_globals) {
+    if (ts->temp_global) {
         pstrcpy(buf, buf_size, ts->name);
     } else if (ts->temp_local) {
         snprintf(buf, buf_size, "loc%d", idx - s->nb_globals);
@@ -1905,7 +1910,7 @@  static void temp_free_or_dead(TCGContext *s, TCGTemp *ts, int free_or_dead)
     }
     ts->val_type = (free_or_dead < 0
                     || ts->temp_local
-                    || temp_idx(s, ts) < s->nb_globals
+                    || ts->temp_global
                     ? TEMP_VAL_MEM : TEMP_VAL_DEAD);
 }
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 70d9fda..3b35344 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -586,10 +586,14 @@  typedef struct TCGTemp {
     unsigned int indirect_base:1;
     unsigned int mem_coherent:1;
     unsigned int mem_allocated:1;
-    unsigned int temp_local:1; /* If true, the temp is saved across
-                                  basic blocks. Otherwise, it is not
-                                  preserved across basic blocks. */
-    unsigned int temp_allocated:1; /* never used for code gen */
+    /* If true, the temp is saved across both basic blocks and
+       translation blocks.  */
+    unsigned int temp_global:1;
+    /* If true, the temp is saved across basic blocks but dead
+       at the end of translation blocks.  If false, the temp is
+       dead at the end of basic blocks.  */
+    unsigned int temp_local:1;
+    unsigned int temp_allocated:1;
 
     tcg_target_long val;
     struct TCGTemp *mem_base;