qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] tcg: optimise memory layout of TCGTemp


From: Laurent Desnogues
Subject: Re: [Qemu-devel] [PATCH] tcg: optimise memory layout of TCGTemp
Date: Mon, 30 Mar 2015 11:55:41 +0200

Hello,

On Fri, Mar 27, 2015 at 10:09 PM, Emilio G. Cota <address@hidden> 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;
>
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]