[Mono-devel-list] AMD64 patches.

[Paolo Molaro]

On 11/14/03 mono_devel at workingpages.com wrote:
> The following patch set should get the interpreter working on AMD64

Nice!

> I chose to make a minimal modification to atomic.h in that the code is
> very close to IA32. Otherwise all the changes should be quite isolated
> in architecture specific places. None of these changes should modify
> the behavior on other platforms.

Yes, it looks nice. I have two suggestions, though:
*) isn't it better to use amd64 as the prefix right away?
x86[-_]64 is the old name and I find amd64 much easier to read in the
code as well.
*) a much more important issue is with the codegen macros and
the inclusion of a copy of x86-codegen.h inside the new header.
There is a lot of code duplication going on here. I think it's best to
#include "x86-codegen.h" inside amd64-codegen.h and provide amd64_xxx
wrappers where needed. I think most of the instructions can just emit
the REX prefix and call the x86_ implementation (with the register
number values properly masked).

> Index: mono/arch/x86_64/x86_64-codegen.h
[...]
> + * Not all routines are done for AMD64. Much could also be removed from here if supporting tramp.c is the only goal.

The goal is to use the file also for the JIT, so we'll need full support
of the instruction set (at least the user-level stuff).

> +#define x86_alu_reg_reg(inst,opc,dreg,reg)	\
> +	do {	\
> +		*(inst)++ = (((unsigned char)(opc)) << 3) + 3;	\
> +		x86_reg_emit ((inst), (dreg), (reg));	\
> +	} while (0)
> +
> +#define x86_64_alu_reg_reg(inst,opc,dreg,reg)	\
> +	do {	\
> +		*(inst)++ = X86_64_REX(X86_64_REX_W); \
> +		*(inst)++ = (((unsigned char)(opc)) << 3) + 3;	\
> +		x86_reg_emit ((inst), (dreg), (reg));	\
> +	} while (0)

Isn't there a bug in this case (I think there are other cases like this
in the file, too)?
The REX prefix may have two additional bit sets, if dreg and/or reg are
bigger than 7 and dreg and reg should be ANDed with 0x3 before passing
them down to x86_reg_emit(), so this should probably look something
like:

#define amd64_alu_reg_reg(inst,opc,dreg,reg)	\
	do {	\
		*(inst)++ = X86_64_REX(X86_64_REX_W| ((dreg) > 7? REX_FLAG1: 0) | ((reg) > 7? REX_FLAG2: 0)); \
		x86_alu_reg_reg ((inst), (opc), (dreg) & 3, (reg) & 3);	\
	} while (0)

Or you can also add the helper macro:

#define amd64_rex_prefix(def64,reg,sib,breg) \
	0x40 | (((def64)? X86_64_REX_W: 0) | (((reg) > 7)? X86_64_REX_R: 0) |
	(((sib) > 7)? X86_64_REX_X: 0) | (((breg) > 7)? X86_64_REX_B: 0))

so it becomes even easier to read:

#define amd64_alu_reg_reg(inst,opc,dreg,reg)	\
	do {	\
		*(inst)++ = amd64_rex_prefix (TRUE, (dreg), (reg), 0);	\
		x86_alu_reg_reg ((inst), (opc), (dreg) & 3, (reg) & 3);	\
	} while (0)

Thanks!

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better