Last Comment Bug 464476 - Enable Tamarin to use Nanojit back-end for x64 builds
: Enable Tamarin to use Nanojit back-end for x64 builds
Status: VERIFIED FIXED
:
Product: Tamarin
Classification: Components
Component: Virtual Machine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Edwin Smith
:
:
Mentors:
Depends on: 468445 474304 475974
Blocks: 469836 476019
  Show dependency treegraph
 
Reported: 2008-11-12 10:00 PST by Rick Reitmaier
Modified: 2009-10-01 04:04 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
adds X64 nanojit backend, enabled for mac & linux, disabled for win (72.86 KB, patch)
2009-01-29 12:31 PST, Edwin Smith
rreitmai: review+
dvander: review+
gal: review+
Details | Diff | Splinter Review
Fix fneg bug, s/pxor/xorps, s/movsdrr/movapdrr (7.08 KB, patch)
2009-02-02 16:55 PST, Edwin Smith
rreitmai: review+
lhansen: review+
Details | Diff | Splinter Review

Description Rick Reitmaier 2008-11-12 10:00:20 PST

    
Comment 1 Rick Reitmaier 2008-12-02 14:23:00 PST
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 2008-12-02 14:26:32 PST
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] 2008-12-02 14:31:11 PST
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 2008-12-03 08:51:33 PST
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 2008-12-03 10:14:21 PST
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] 2008-12-03 10:17:20 PST
(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 2009-01-27 16:55:51 PST

On 1/27/09 7:27 PM, "Rick Reitmaier" <rreitmai@adobe.com> 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 2009-01-29 12:31:46 PST
Created attachment 359585 [details] [diff] [review]
adds X64 nanojit backend, enabled for mac & linux, disabled for win
Comment 9 Rick Reitmaier 2009-01-30 11:16:34 PST
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 2009-01-30 13:11:10 PST
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 2009-01-30 13:13:52 PST
(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 2009-01-30 13:16:09 PST
(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 2009-02-02 16:26:59 PST
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 2009-02-02 16:55:38 PST
Created attachment 360198 [details] [diff] [review]
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 2009-02-03 09:04:05 PST
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 2009-02-03 09:06:53 PST
haven't hit that case yet, but i can force it to be used if PEDANTIC=1
Comment 17 Lars T Hansen 2009-02-03 23:26:24 PST
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 2009-02-04 05:41:39 PST
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

Note You need to log in before you can comment on or make changes to this bug.