The default bug view has changed. See this FAQ.

Enable Tamarin to use Nanojit back-end for x64 builds

VERIFIED FIXED

Status

Tamarin
Virtual Machine
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: Rick Reitmaier, Assigned: Edwin Smith)

Tracking

Details

Attachments

(2 obsolete attachments)

Comment hidden (empty)
Depends on: 463743
(Reporter)

Comment 1

8 years ago
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

8 years ago
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.)
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).
(Assignee)

Comment 4

8 years ago
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.
(Assignee)

Comment 5

8 years ago
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.
(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.
(Assignee)

Updated

8 years ago
Depends on: 468445
(Reporter)

Updated

8 years ago
Assignee: rreitmai → edwsmith

Updated

8 years ago
Blocks: 469836
(Assignee)

Updated

8 years ago
Depends on: 474304
(Assignee)

Updated

8 years ago
Depends on: 464643
(Assignee)

Comment 7

8 years ago

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
(Assignee)

Updated

8 years ago
No longer depends on: 464643
(Assignee)

Updated

8 years ago
Depends on: 475974
(Assignee)

Comment 8

8 years ago
Created attachment 359585 [details] [diff] [review]
adds X64 nanojit backend, enabled for mac & linux, disabled for win
Attachment #359585 - Flags: review?(rreitmai)
Attachment #359585 - Flags: review?(gal)
Attachment #359585 - Flags: review?(danderson)
(Assignee)

Updated

8 years ago
Blocks: 476019
(Reporter)

Updated

8 years ago
Attachment #359585 - Flags: review?(rreitmai) → review+
(Reporter)

Comment 9

8 years ago
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
(Assignee)

Updated

8 years ago
No longer depends on: 463743

Updated

8 years ago
Attachment #359585 - Flags: review?(gal) → review+

Comment 10

8 years ago
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.
(Assignee)

Comment 11

8 years ago
(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
(Assignee)

Comment 12

8 years ago
(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.
Attachment #359585 - Flags: review?(danderson) → review+
(Assignee)

Updated

8 years ago
Attachment #359585 - Attachment is obsolete: true
(Assignee)

Comment 13

8 years ago
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
(Assignee)

Comment 14

8 years ago
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).
Attachment #360198 - Flags: review?(rreitmai)
(Assignee)

Updated

8 years ago
Attachment #360198 - Flags: review?(lhansen)
(Reporter)

Comment 15

8 years ago
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.
Attachment #360198 - Flags: review?(rreitmai) → review+
(Assignee)

Comment 16

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

Updated

8 years ago
Attachment #360198 - Flags: review?(lhansen) → review+

Comment 17

8 years ago
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.
(Assignee)

Comment 18

8 years ago
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
(Assignee)

Updated

8 years ago
Attachment #360198 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.