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;
