| Message ID | 1437755447-10537-4-git-send-email-aurelien@aurel32.net |
|---|---|
| State | New |
| Headers | show |
On 24/07/2015 18:30, Aurelien Jarno wrote: > Use two bools to track constants and copies instead of an enum. > > Cc: Richard Henderson <rth@twiddle.net> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > --- > tcg/optimize.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/tcg/optimize.c b/tcg/optimize.c > index d2b63a4..f16eb1e 100644 > --- a/tcg/optimize.c > +++ b/tcg/optimize.c > @@ -35,14 +35,9 @@ > glue(glue(case INDEX_op_, x), _i32): \ > glue(glue(case INDEX_op_, x), _i64) > > -typedef enum { > - TCG_TEMP_UNDEF = 0, > - TCG_TEMP_CONST, > - TCG_TEMP_COPY, > -} tcg_temp_state; > - > struct tcg_temp_info { > - tcg_temp_state state; > + bool is_const; > + bool is_copy; Could temps[arg].is_copy be replaced by temps[arg].next_copy != arg? For example, this: if (temps[temp].prev_copy == temps[temp].next_copy) { temps[temps[temp].next_copy].is_copy = false; } else { temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; } would be replaced simply by temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; Paolo > uint16_t prev_copy; > uint16_t next_copy; > tcg_target_ulong val; > @@ -54,12 +49,12 @@ static TCGTempSet temps_used; > > static inline bool temp_is_const(TCGArg arg) > { > - return temps[arg].state == TCG_TEMP_CONST; > + return temps[arg].is_const; > } > > static inline bool temp_is_copy(TCGArg arg) > { > - return temps[arg].state == TCG_TEMP_COPY; > + return temps[arg].is_copy; > } > > /* Reset TEMP's state to TCG_TEMP_UNDEF. If TEMP only had one copy, remove > @@ -68,13 +63,14 @@ static void reset_temp(TCGArg temp) > { > if (temp_is_copy(temp)) { > if (temps[temp].prev_copy == temps[temp].next_copy) { > - temps[temps[temp].next_copy].state = TCG_TEMP_UNDEF; > + temps[temps[temp].next_copy].is_copy = false; > } else { > temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; > temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; > } > } > - temps[temp].state = TCG_TEMP_UNDEF; > + temps[temp].is_const = false; > + temps[temp].is_copy = false; > temps[temp].mask = -1; > } > > @@ -88,7 +84,8 @@ static void reset_all_temps(int nb_temps) > static void init_temp_info(TCGArg temp) > { > if (!test_bit(temp, temps_used.l)) { > - temps[temp].state = TCG_TEMP_UNDEF; > + temps[temp].is_const = false; > + temps[temp].is_copy = false; > temps[temp].mask = -1; > set_bit(temp, temps_used.l); > } > @@ -218,7 +215,8 @@ static void tcg_opt_gen_movi(TCGContext *s, TCGOp *op, TCGArg *args, > op->opc = new_op; > > reset_temp(dst); > - temps[dst].state = TCG_TEMP_CONST; > + temps[dst].is_const = true; > + temps[dst].is_copy = false; > temps[dst].val = val; > mask = val; > if (TCG_TARGET_REG_BITS > 32 && new_op == INDEX_op_movi_i32) { > @@ -261,11 +259,13 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, > > if (s->temps[src].type == s->temps[dst].type) { > if (!temp_is_copy(src)) { > - temps[src].state = TCG_TEMP_COPY; > + temps[src].is_const = false; > + temps[src].is_copy = true; > temps[src].next_copy = src; > temps[src].prev_copy = src; > } > - temps[dst].state = TCG_TEMP_COPY; > + temps[dst].is_const = false; > + temps[dst].is_copy = true; > temps[dst].next_copy = temps[src].next_copy; > temps[dst].prev_copy = src; > temps[temps[dst].next_copy].prev_copy = dst; >
On 2015-07-27 10:25, Paolo Bonzini wrote: > > > On 24/07/2015 18:30, Aurelien Jarno wrote: > > Use two bools to track constants and copies instead of an enum. > > > > Cc: Richard Henderson <rth@twiddle.net> > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > > --- > > tcg/optimize.c | 30 +++++++++++++++--------------- > > 1 file changed, 15 insertions(+), 15 deletions(-) > > > > diff --git a/tcg/optimize.c b/tcg/optimize.c > > index d2b63a4..f16eb1e 100644 > > --- a/tcg/optimize.c > > +++ b/tcg/optimize.c > > @@ -35,14 +35,9 @@ > > glue(glue(case INDEX_op_, x), _i32): \ > > glue(glue(case INDEX_op_, x), _i64) > > > > -typedef enum { > > - TCG_TEMP_UNDEF = 0, > > - TCG_TEMP_CONST, > > - TCG_TEMP_COPY, > > -} tcg_temp_state; > > - > > struct tcg_temp_info { > > - tcg_temp_state state; > > + bool is_const; > > + bool is_copy; > > Could temps[arg].is_copy be replaced by temps[arg].next_copy != arg? > > For example, this: > > if (temps[temp].prev_copy == temps[temp].next_copy) { > temps[temps[temp].next_copy].is_copy = false; > } else { > temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; > temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; > } > > would be replaced simply by > > temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; > temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; > That's an interesting idea, especially I have a patch series in preparation that get rid of is_const. I will try to get something like that for v2.
Aurelien Jarno <aurelien@aurel32.net> writes: > Use two bools to track constants and copies instead of an enum. More of an explanation would be useful as to why, otherwise it seems to me we are just increasing the size of the structure (assuming the compiler uses the same base sizes for the bool and enum). > > Cc: Richard Henderson <rth@twiddle.net> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> > --- > tcg/optimize.c | 30 +++++++++++++++--------------- > 1 file changed, 15 insertions(+), 15 deletions(-) > > diff --git a/tcg/optimize.c b/tcg/optimize.c > index d2b63a4..f16eb1e 100644 > --- a/tcg/optimize.c > +++ b/tcg/optimize.c > @@ -35,14 +35,9 @@ > glue(glue(case INDEX_op_, x), _i32): \ > glue(glue(case INDEX_op_, x), _i64) > > -typedef enum { > - TCG_TEMP_UNDEF = 0, > - TCG_TEMP_CONST, > - TCG_TEMP_COPY, > -} tcg_temp_state; > - > struct tcg_temp_info { > - tcg_temp_state state; > + bool is_const; > + bool is_copy; > uint16_t prev_copy; > uint16_t next_copy; > tcg_target_ulong val; > @@ -54,12 +49,12 @@ static TCGTempSet temps_used; > > static inline bool temp_is_const(TCGArg arg) > { > - return temps[arg].state == TCG_TEMP_CONST; > + return temps[arg].is_const; > } > > static inline bool temp_is_copy(TCGArg arg) > { > - return temps[arg].state == TCG_TEMP_COPY; > + return temps[arg].is_copy; > } > > /* Reset TEMP's state to TCG_TEMP_UNDEF. If TEMP only had one copy, remove > @@ -68,13 +63,14 @@ static void reset_temp(TCGArg temp) > { > if (temp_is_copy(temp)) { > if (temps[temp].prev_copy == temps[temp].next_copy) { > - temps[temps[temp].next_copy].state = TCG_TEMP_UNDEF; > + temps[temps[temp].next_copy].is_copy = false; > } else { > temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; > temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; > } > } > - temps[temp].state = TCG_TEMP_UNDEF; > + temps[temp].is_const = false; > + temps[temp].is_copy = false; > temps[temp].mask = -1; > } > > @@ -88,7 +84,8 @@ static void reset_all_temps(int nb_temps) > static void init_temp_info(TCGArg temp) > { > if (!test_bit(temp, temps_used.l)) { > - temps[temp].state = TCG_TEMP_UNDEF; > + temps[temp].is_const = false; > + temps[temp].is_copy = false; > temps[temp].mask = -1; > set_bit(temp, temps_used.l); > } > @@ -218,7 +215,8 @@ static void tcg_opt_gen_movi(TCGContext *s, TCGOp *op, TCGArg *args, > op->opc = new_op; > > reset_temp(dst); > - temps[dst].state = TCG_TEMP_CONST; > + temps[dst].is_const = true; > + temps[dst].is_copy = false; > temps[dst].val = val; > mask = val; > if (TCG_TARGET_REG_BITS > 32 && new_op == INDEX_op_movi_i32) { > @@ -261,11 +259,13 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, > > if (s->temps[src].type == s->temps[dst].type) { > if (!temp_is_copy(src)) { > - temps[src].state = TCG_TEMP_COPY; > + temps[src].is_const = false; > + temps[src].is_copy = true; > temps[src].next_copy = src; > temps[src].prev_copy = src; > } > - temps[dst].state = TCG_TEMP_COPY; > + temps[dst].is_const = false; > + temps[dst].is_copy = true; > temps[dst].next_copy = temps[src].next_copy; > temps[dst].prev_copy = src; > temps[temps[dst].next_copy].prev_copy = dst;
On 2015-07-29 17:10, Alex Bennée wrote: > > Aurelien Jarno <aurelien@aurel32.net> writes: > > > Use two bools to track constants and copies instead of an enum. > > More of an explanation would be useful as to why, otherwise it seems to > me we are just increasing the size of the structure (assuming the > compiler uses the same base sizes for the bool and enum). The reason is in the following patch, to allow separate tracking of const and copy. Note that this doesn't increase the structure size due to alignment and that the v2 of this patchset actually uses only one bool. I'll add a note about why it is need for v3.
diff --git a/tcg/optimize.c b/tcg/optimize.c index d2b63a4..f16eb1e 100644 --- a/tcg/optimize.c +++ b/tcg/optimize.c @@ -35,14 +35,9 @@ glue(glue(case INDEX_op_, x), _i32): \ glue(glue(case INDEX_op_, x), _i64) -typedef enum { - TCG_TEMP_UNDEF = 0, - TCG_TEMP_CONST, - TCG_TEMP_COPY, -} tcg_temp_state; - struct tcg_temp_info { - tcg_temp_state state; + bool is_const; + bool is_copy; uint16_t prev_copy; uint16_t next_copy; tcg_target_ulong val; @@ -54,12 +49,12 @@ static TCGTempSet temps_used; static inline bool temp_is_const(TCGArg arg) { - return temps[arg].state == TCG_TEMP_CONST; + return temps[arg].is_const; } static inline bool temp_is_copy(TCGArg arg) { - return temps[arg].state == TCG_TEMP_COPY; + return temps[arg].is_copy; } /* Reset TEMP's state to TCG_TEMP_UNDEF. If TEMP only had one copy, remove @@ -68,13 +63,14 @@ static void reset_temp(TCGArg temp) { if (temp_is_copy(temp)) { if (temps[temp].prev_copy == temps[temp].next_copy) { - temps[temps[temp].next_copy].state = TCG_TEMP_UNDEF; + temps[temps[temp].next_copy].is_copy = false; } else { temps[temps[temp].next_copy].prev_copy = temps[temp].prev_copy; temps[temps[temp].prev_copy].next_copy = temps[temp].next_copy; } } - temps[temp].state = TCG_TEMP_UNDEF; + temps[temp].is_const = false; + temps[temp].is_copy = false; temps[temp].mask = -1; } @@ -88,7 +84,8 @@ static void reset_all_temps(int nb_temps) static void init_temp_info(TCGArg temp) { if (!test_bit(temp, temps_used.l)) { - temps[temp].state = TCG_TEMP_UNDEF; + temps[temp].is_const = false; + temps[temp].is_copy = false; temps[temp].mask = -1; set_bit(temp, temps_used.l); } @@ -218,7 +215,8 @@ static void tcg_opt_gen_movi(TCGContext *s, TCGOp *op, TCGArg *args, op->opc = new_op; reset_temp(dst); - temps[dst].state = TCG_TEMP_CONST; + temps[dst].is_const = true; + temps[dst].is_copy = false; temps[dst].val = val; mask = val; if (TCG_TARGET_REG_BITS > 32 && new_op == INDEX_op_movi_i32) { @@ -261,11 +259,13 @@ static void tcg_opt_gen_mov(TCGContext *s, TCGOp *op, TCGArg *args, if (s->temps[src].type == s->temps[dst].type) { if (!temp_is_copy(src)) { - temps[src].state = TCG_TEMP_COPY; + temps[src].is_const = false; + temps[src].is_copy = true; temps[src].next_copy = src; temps[src].prev_copy = src; } - temps[dst].state = TCG_TEMP_COPY; + temps[dst].is_const = false; + temps[dst].is_copy = true; temps[dst].next_copy = temps[src].next_copy; temps[dst].prev_copy = src; temps[temps[dst].next_copy].prev_copy = dst;
Use two bools to track constants and copies instead of an enum. Cc: Richard Henderson <rth@twiddle.net> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net> --- tcg/optimize.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-)