464476 - Enable Tamarin to use Nanojit back-end for x64 builds

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect)

Comment 1

[Rick Reitmaier]

Ok, I've worked through a number of issues related to lir generation (patch soon to be posted for TM folks) and have hit a rather painful spot related to tamarin only.

vm_fops.h contains the CallInfo table that CodegenLIR uses.  For debugging purposes, the signature is embedded in the table and pointers to methods are encoded as argtype LO => implies 32bits.  

I'm guessing the right solution is to introduce a new argtype PTR and fix up the table, but wanted to get other opinions before I start this heroic effort.

Comment 2

[Steven Johnson]

Ow... yeah, I think that is probably going to a painful necessity. (It might also mean you have to expand the size of the bitfield to 3 bits.)

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

If I'm thinking of the same thing -- In TraceMonkey we have a PTR define that maps to either Q or LO.  We did have to go through and catch all the CallInfo cases where this mattered.  Luckily that's only return values, as all arguments are passed in 64-bit wide slots on AMD64 (though perhaps for real portability we should specify it on parameters as well).

Comment 4

[Edwin Smith]

just to complicate matters, ARGTYPE_Q implies that the arg is a 32-bit reference to a 64bit value, which isnt the same as being a 64bit value arg.

I think all the 64bit ABI's would expand 32bit params to 64bit.  so, ARGTYPE_LO can really mean ARGTYPE_W (machine word size).  any 32bit ints being passed as params would have to be sign or zero extended to 64bit anyway, to satisfy the 64bit ABI.

so i think we just need to rename ARGTYPE_LO to imply stack-width arguments, (s/ARGTYPE_LO/ARGTYPE_W), keep ARGTYPE_F to imply 64bit double arguments, and keep ARGTYPE_Q to imply word-size-ref-to-64bit arg.

Comment 5

[Edwin Smith]

if ARGTYPE_W implies 64bit non-double, then i think asm_call should *require* the argument LIns to be isQuad(), and its up to the code generator to sign-extend or zero-extend any 32bit values.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to comment #5)
> if ARGTYPE_W implies 64bit non-double, then i think asm_call should *require*
> the argument LIns to be isQuad(), and its up to the code generator to
> sign-extend or zero-extend any 32bit values.

I agree.  LO -> W sounds good as well.

Comment 7

[Edwin Smith]

On 1/27/09 7:27 PM, "Rick Reitmaier" <[e-mail]> wrote:

Shouldn't this be ifdef AVMPLUS_I32 ?    Don't all AMD64's support sse2?

        #if defined (AVMPLUS_IA32) || defined(AVMPLUS_AMD64)
        bool sse2;
        bool use_cmov;
        #endif

Comment 8

[Edwin Smith]

Attachment: adds X64 nanojit backend, enabled for mac & linux, disabled for win

Comment 9

[Rick Reitmaier]

Comment on attachment 359585 [details] [diff] [review]
adds X64 nanojit backend, enabled for mac & linux, disabled for win

avmbuild.h - defs duplicated
Assembler.cpp - x64 and amd64 coexist for the moment only, correct? 
CodegenLIR.cpp - nit; emit() uses int should it be int_t

Comment 10

[Andreas Gal :gal]

Comment on attachment 359585 [details] [diff] [review]
adds X64 nanojit backend, enabled for mac & linux, disabled for win

Looks good. I have a patch to add idiv and mod. Want to add that while you are at it. Also, I can do asm_loop adn nFragExit. Shouldn't be too hard. We do it as we merge.

Comment 11

[Edwin Smith]

(In reply to comment #9)
> (From update of attachment 359585 [details] [diff] [review])
> avmbuild.h - defs duplicated

I'm not sure which def's you're referring to?

> Assembler.cpp - x64 and amd64 coexist for the moment only, correct? 

correct

> CodegenLIR.cpp - nit; emit() uses int should it be int_t

fixed

Comment 12

[Edwin Smith]

(In reply to comment #10)
> (From update of attachment 359585 [details] [diff] [review])
> Looks good. I have a patch to add idiv and mod. Want to add that while you are
> at it. Also, I can do asm_loop adn nFragExit. Shouldn't be too hard. We do it
> as we merge.

I will create a bug to add these after this lands (or should we keep your bug open? either is fine)  you'll also notice that this selector doesn't choose immediate-mode instructions or short-displacement forms, that's also coming in a followon patch.

besides keeping patch size down, it'll be interesting to see how much these affect size and performance; that's hard to tell once the code is all tangled up.

Comment 13

[Edwin Smith]

Comment on attachment 359585 [details] [diff] [review]
adds X64 nanojit backend, enabled for mac & linux, disabled for win

pushed
http://hg.mozilla.org/tamarin-redux/rev/9d4b7ee595d4

Comment 14

[Edwin Smith]

Attachment: Fix fneg bug, s/pxor/xorps, s/movsdrr/movapdrr

Fix for fneg to use xorps, 0-x is not correct.  Use xorps instead of pxor for in conjunction with floating point ops to avoid cross-domain stalls.  Use movaps instead of movsd for xmm<-->xmm copies to avoid partial register stalls.  (credit to Mike Pall for these suggestions).

Comment 15

[Rick Reitmaier]

Comment on attachment 360198 [details] [diff] [review]
Fix fneg bug, s/pxor/xorps, s/movsdrr/movapdrr

I suspect that the 'this is just hideous' comment refers to some code that follows.

Comment 16

[Edwin Smith]

haven't hit that case yet, but i can force it to be used if PEDANTIC=1

Comment 17

[Lars T Hansen]

Comment on attachment 360198 [details] [diff] [review]
Fix fneg bug, s/pxor/xorps, s/movsdrr/movapdrr

nits, first comment says XORPD but code says XORPS, also first word of last line of first comment is misspelled.

Comment 18

[Edwin Smith]

Comment on attachment 360198 [details] [diff] [review]
Fix fneg bug, s/pxor/xorps, s/movsdrr/movapdrr

pushed http://hg.mozilla.org/tamarin-redux/rev/52c9b4654f18

comment fixed in http://hg.mozilla.org/tamarin-redux/rev/a8359aa48027