From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57198) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbbVU-0004Zz-5I for qemu-devel@nongnu.org; Fri, 27 Mar 2015 17:09:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YbbVN-0004Rb-Ne for qemu-devel@nongnu.org; Fri, 27 Mar 2015 17:09:32 -0400 Received: from out2-smtp.messagingengine.com ([66.111.4.26]:48366) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YbbVN-0004RR-Ir for qemu-devel@nongnu.org; Fri, 27 Mar 2015 17:09:25 -0400 Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id 4892A20927 for ; Fri, 27 Mar 2015 17:09:21 -0400 (EDT) Date: Fri, 27 Mar 2015 17:09:32 -0400 From: "Emilio G. Cota" Message-ID: <20150327210932.GA17458@flamenco> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <55156FFE.9050806@twiddle.net> <87y4mibw94.fsf@linaro.org> Subject: Re: [Qemu-devel] [PATCH] tcg: optimise memory layout of TCGTemp List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex =?iso-8859-1?Q?Benn=E9e?= , Richard Henderson Cc: qemu-trivial@nongnu.org, Stefan Weil , qemu-devel@nongnu.org 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. > > 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;