| Message ID | 20170621024831.26019-7-rth@twiddle.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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~
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
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;
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(-)