Hello, On Fri, Mar 27, 2015 at 10:09 PM, Emilio G. Cota <c...@braap.org> wrote: > On Fri, Mar 27, 2015 at 09:55:03 +0000, Alex Bennée wrote: >> Have you been able to measure any performance improvement with these new >> structures? In theory, if aligned with cache lines, performance should >> improve but real numbers would be nice. > > I haven't benchmarked anything, which makes me very uneasy. All > I've checked is that the system boots, and FWIW I appreciate no > difference in boot time. > > Is there a benchmark suite to test TCG changes? > > Until proper benchmarking I wouldn't want to see this merged. For now I > propose to merge the initial change (remove 8-byte hole in 64-bit), > which is uncontroversial.
I tested the patch attached to your mail and saw no performance difference on an ARM image booting Linux and then running Sunspider with Google v8. I also tested on one of the 176.gcc inputs with QEMU ARM user mode and again saw no difference. Thanks, Laurent >> > The appended adds macros to prevent us from mistakenly overflowing >> > the bitfields when more elements are added to the corresponding >> > enums/macros. >> >> I can see the defines but I can't see any checks. Should we be able to >> do a compile time check if TCG_TYPE_COUNT doesn't fit into >> TCG_TYPE_NR_BITS? > >> > +#define TEMP_VAL_NR_BITS 2 >> >> A similar compile time check could be added here. > > Ack, addressed below. > > On Fri, Mar 27, 2015 at 07:58:06 -0700, Richard Henderson wrote: >> On 03/25/2015 12:50 PM, Emilio G. Cota wrote: >> > +#define TCG_TYPE_NR_BITS 1 >> >> I'd rather you moved TCG_TYPE_COUNT out of the enum and into a define. >> Perhaps >> even as (1 << TCG_TYPE_NR_BITS). > (snip) >> > +#define TEMP_VAL_NR_BITS 2 >> >> And make this an enumeration. >> >> > typedef struct TCGTemp { > (snip) >> > + unsigned int base_type:TCG_TYPE_NR_BITS; >> > + unsigned int type:TCG_TYPE_NR_BITS; >> >> And do *not* change these from the enumeration to an unsigned int. >> >> I know why you did this -- to keep the compiler from warning that the TCGType >> enum didn't fit in the bitfield, because of TCG_TYPE_COUNT being an >> enumerator, >> rather than an unrelated number. Except that's exactly the warning we want >> to >> keep, on the off-chance that someone modifies the enums without modifying the >> _NR_BITS defines. > > Agreed, please see below. > > Thanks, > > E. > > [No signoff due to lack of provable perf improvement, see above.] > > diff --git a/tcg/tcg.h b/tcg/tcg.h > index add7f75..afd3f94 100644 > --- a/tcg/tcg.h > +++ b/tcg/tcg.h > @@ -193,7 +193,6 @@ typedef struct TCGPool { > typedef enum TCGType { > TCG_TYPE_I32, > TCG_TYPE_I64, > - TCG_TYPE_COUNT, /* number of different types */ > > /* An alias for the size of the host register. */ > #if TCG_TARGET_REG_BITS == 32 > @@ -217,6 +216,10 @@ typedef enum TCGType { > #endif > } TCGType; > > +/* used for bitfield packing to save space */ > +#define TCG_TYPE_NR_BITS 1 > +#define TCG_TYPE_COUNT BIT(TCG_TYPE_NR_BITS) > + > /* Constants for qemu_ld and qemu_st for the Memory Operation field. */ > typedef enum TCGMemOp { > MO_8 = 0, > @@ -417,20 +420,21 @@ static inline TCGCond tcg_high_cond(TCGCond c) > } > } > > -#define TEMP_VAL_DEAD 0 > -#define TEMP_VAL_REG 1 > -#define TEMP_VAL_MEM 2 > -#define TEMP_VAL_CONST 3 > +typedef enum TCGTempVal { > + TEMP_VAL_DEAD, > + TEMP_VAL_REG, > + TEMP_VAL_MEM, > + TEMP_VAL_CONST, > +} TCGTempVal; > + > +#define TEMP_VAL_NR_BITS 2 > > -/* XXX: optimize memory layout */ > typedef struct TCGTemp { > - TCGType base_type; > - TCGType type; > - int val_type; > - int reg; > - tcg_target_long val; > - int mem_reg; > - intptr_t mem_offset; > + unsigned int reg:8; > + unsigned int mem_reg:8; > + TCGTempVal val_type:TEMP_VAL_NR_BITS; > + TCGType base_type:TCG_TYPE_NR_BITS; > + TCGType type:TCG_TYPE_NR_BITS; > unsigned int fixed_reg:1; > unsigned int mem_coherent:1; > unsigned int mem_allocated:1; > @@ -438,6 +442,9 @@ typedef struct TCGTemp { > basic blocks. Otherwise, it is not > preserved across basic blocks. */ > unsigned int temp_allocated:1; /* never used for code gen */ > + > + tcg_target_long val; > + intptr_t mem_offset; > const char *name; > } TCGTemp; > >