[Mono-dev] JIT: MonoReg to replace gssize for registers

[Mark Mason]

Hello Andreas,

Sorry about the line-wrap. I'm still getting the hang of evolution as a
mailer.

Good catch on the off-by-one. Thanks - it works a lot better now ;). I'd
flipped this bit of code in and out a few times while trying different
approaches, and got it wrong the last time it went in :(.

There may be other endian-gotchas as well (I occasionally find
little-endian assumptions in the code -- see the patch I posted recently
for trace.c for an unrelated example), but this is the only case
involving introducing mgreg_t that I've specifically tripped over so
far. Namely, PCONST really being ICONST, and the subsequent type-punning
between inst_p0 (assigned) and inst_c0 (referenced).

Some of my earlier attempts to address this involved making 'p' itself a
mgreg_t, but there's too much of an assumption that inst_p0 and inst_p1
are really pointer types to keep it from being highly invasive.

/Mark

On Mon, 2008-12-15 at 23:48 +0100, Andreas Färber wrote:
> Hello,
> 
> Am 15.12.2008 um 23:30 schrieb Mark Mason:
> 
> > On Sun, 2008-12-14 at 10:13 +0100, Zoltan Varga wrote:
> >>  There is already a type for this called 'greg_t' on linux. So it
> >> might be useful to name the
> >> new type something similar, like mgreg_t, or something. Other than
> >> that, based on the
> >> discussion, I think this is ok to check in.
> >
> > With this in mind, here is the updated patch. I'm still not entirely
> > comfortable with the type aliasing between inst_p* and inst_c*, but I
> > think this will come the closest to doing the right thing with minimal
> > changes.
> >
> > /Mark
> >
> > Index: mini.h
> > ===================================================================
> > --- mini.h	(revision 121486)
> > +++ mini.h	(working copy)
> > @@ -400,6 +400,16 @@
> > 	int size, align;
> > } MonoMemcpyArgs;
> >
> > +/* C type matching the size of a machine register. Not always the  
> > same
> > as 'int' */
> > +/* Note that member 'p' of MonoInst must be the same type, as  
> > OP_PCONST
> > is defined
> > + * as one of the OP_ICONST types, so inst_c0 must be the same as
> > inst_p0
> 
> Line wrapped.
> 
> > + */
> > +#if SIZEOF_REGISTER == 4
> > +typedef gint32 mgreg_t;
> > +#elif SIZEOF_REGISTER == 8
> > +typedef gint64 mgreg_t;
> > +#endif
> > +
> > struct MonoInst {
> >  	guint16 opcode;
> > 	guint8  type; /* stack type */
> > @@ -415,8 +425,14 @@
> > 		union {
> > 			MonoInst *src;
> > 			MonoMethodVar *var;
> > -			gssize const_val;
> > +			mgreg_t const_val;
> > +#if (SIZEOF_REGISTER > SIZEOF_VOID_P) && (G_BYTE_ORDER ==  
> > G_BIG_ENDIAN)
> > +			struct {
> > +				gpointer p[SIZEOF_REGISTER/SIZEOF_VOID_P];
> > +			} pdata;
> > +#else
> > 			gpointer p;
> > +#endif
> > 			MonoMethod *method;
> > 			MonoMethodSignature *signature;
> > 			MonoBasicBlock **many_blocks;
> 
> What does endianess have to do with register size?
> 
> > @@ -511,8 +527,13 @@
> > #define inst_c1 data.op[1].const_val
> > #define inst_i0 data.op[0].src
> > #define inst_i1 data.op[1].src
> > +#if (SIZEOF_REGISTER > SIZEOF_VOID_P) && (G_BYTE_ORDER ==  
> > G_BIG_ENDIAN)
> > +#define inst_p0 data.op[0].pdata.p[SIZEOF_REGISTER/SIZEOF_VOID_P]
> > +#define inst_p1 data.op[1].pdata.p[SIZEOF_REGISTER/SIZEOF_VOID_P]
> 
> You are off by one.
> 
> Is this not the only place where endianess would matter, i.e. first  
> index vs. last index?
> 
> > +#else
> > #define inst_p0 data.op[0].p
> > #define inst_p1 data.op[1].p
> > +#endif
> > #define inst_l  data.i8const
> > #define inst_r  data.r8const
> > #define inst_left  data.op[0].src
> 
> Andreas
>