Integrate sparc nanojit intro tracemonkey

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: wes, Assigned: leon.sha)

Tracking

({fixed1.9.1})

unspecified
Sun
Solaris
fixed1.9.1
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(7 attachments)

(Reporter)

Description

10 years ago
- I started with Leon Sha's Tamarin patch in bug 458060
 - Tweaked build system to build JIT when CPU is sparc
 - Sorted out the API differences by comparing the ARM code in tracemonkey and tamarin-central
   - placeGuardRecord became guard->record
   - Assembler::assignSavedParams became Assembler::assignSavedRegs
   - guard->exit() became guard->record()->exit
   - nPatchBranch was added and based on asm_adjustBranch
   - asm_branch modified to accept and ignore far parameter
   - asm_adjustBranch and asm_ret were removed
   - naming adjusted to stay consistent with rest of tree (sparc became Sparc)

I'm building with GCC although I left Leon's Sun Studio fixes in.

Trivial tests indicate that the JIT is working.  For example, iterating over large empty loops takes much less time (6.3 vs. 0.1 wall-clock seconds for 1 million iterations).

Trace-test.js has a few failures, but at least some of them have nothing to do with tracing. 

Tests which fail which might be related to tracing:

matchInLoop,testReplace2,testRegExpTest
recorder: started(1347), aborted(423), completed(1380), different header(14), trees trashed(4), slot promoted(0), 
unstable loop variable(132), breaks(2), returns(15), unstableInnerCalls(70)
monitor: triggered(367312), exits(367074), type mismatch(0), global mismatch(92)
(Reporter)

Comment 1

10 years ago
Created attachment 359627 [details] [diff] [review]
Initial patch to add tracing on sparc
(Reporter)

Updated

10 years ago
Attachment #359627 - Attachment is patch: true
Attachment #359627 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Comment 2

10 years ago
Further digging and help from bsmedberg indicates that current jit-mode test failures are related to bug 462027 and not to this patch.

So, I think once 462027 is fixed trace-test.js will run through on sparc just fine.
Depends on: 462027

Comment 3

10 years ago
The blocking bug was fixed and just fixed a similar issue with ARM. Please re-test to confirm that this also fixes the sparc issue. Thanks!
(Assignee)

Comment 4

10 years ago
These three failures is not related to bug 462027. It is a bug in nanojit sparc code. I've fixed two of them and I am still working the last one.
(Reporter)

Comment 5

10 years ago
Hi, Leon;

Thanks for looking into this, I was about to email you to ask for help.

I found yesterday that my test case (a simple String.replace()) is segfaulting in native code while executing a regular expression. The crash happens on an instruction which gdb decodes as "unknown".

If you need a guinea pig/gopher/anything, feel free to ping me in email or IRC, I am happy to help. Having sparc nanojit available in tracemonkey is a really big deal!

FWIW -- my test program crashes at 0xfefddf90
0xfefddf60:	nop 
0xfefddf64:	nop 
0xfefddf68:	nop 
0xfefddf6c:	save  %sp, -152, %sp
0xfefddf70:	unknown
0xfefddf74:	unknown
0xfefddf78:	cmp  %l1, %l3
0xfefddf7c:	bg  0xfefddfbc
0xfefddf80:	nop 
0xfefddf84:	cmp  %l1, %l3
0xfefddf88:	bge  0xfefddfc8
0xfefddf8c:	nop 
0xfefddf90:	unknown
0xfefddf94:	mov  0x20, %l0
0xfefddf98:	cmp  %l3, %l0
0xfefddf9c:	bne  0xfefddfc8
0xfefddfa0:	nop 

   
    fefddf60  [prologue]
              nop
              save %sp, -152, %sp
        patch entry:
        fefddf68:
patching fefddfd4 to fefddf68
        fefddf68:
compiling trunk 1c3de8 T1
    state = param 0 %i0
    gdata = param 1 %i1 
    start = ld gdata[16]
    cpend = ld gdata[24]
    jf le1 -> label1
              ld [%i1 + 16], %l1              %i0(state) %i1(gdata)
              ld [%i1 + 24], %l3              %l1(start) %i0(state) %i1(gdata)
              subcc %l1, %l3, %g0             %l1(start) %l3(cpend) %i0(state) %i1(gdata)
              bg fefddfb4                     %l1(start) %l3(cpend) %i0(state) %i1(gdata)
              nop                             %l1(start) %l3(cpend) %i0(state) %i1(gdata)
    jf lt1 -> label2
              subcc %l1, %l3, %g0             %l1(start) %l3(cpend) %i0(state) %i1(gdata)
              bge fefddfc0                    %l1(start) %i0(state) %i1(gdata)
             nop                             %l1(start) %i0(state) %i1(gdata)
    ldcs1 = ldcs start[0]
    jf eq1 -> label2
              ld [%l1 + 0], %l3               %l1(start) %i0(state) %i1(gdata)
              or %g0, 32, %l0                 %l1(start) %l3(ldcs1) %i0(state) %i1(gdata)
              subcc %l3, %l0, %g0             %l1(start) %l3(ldcs1) %i0(state) %i1(gdata)
              bne fefddfc0                    %l1(start) %i0(state) %i1(gdata)
              nop                             %l1(start) %i0(state) %i1(gdata)
              or %i0, 0, %o0                  %l1(start) %i0(state)
    add1 = add start, 2
    sti state[0] = add1
    ret state
              or %g0, 2, %l0                  %o0(state) %l1(start) 
              add %l1, %l0, %l1               %o0(state) %l1(start)
              st %l1, [%o0 + 0]               %o0(state) %l1(add1)
              ba fefddfe4
              nop
    label1:
    0
    ret 0
    fefddfb4  [label1]  
              xor %o0, %o0, %o0
              ba fefddfe4                     %l1(start) %i0(state) %i1(gdata)
              nop                             %l1(start) %i0(state) %i1(gdata)
    label2:
    add2 = add start, 2
    sti gdata[16] = add2
    1
    loop
    fefddfc0  [label2]                        %l1(start) %i0(state) %i1(gdata)
              or %g0, 2, %l0                  %l1(start) %i0(state) %i1(gdata)
              add %l1, %l0, %l1               %l1(start) %i0(state) %i1(gdata)
              st %l1, [%i1 + 16]              %l1(add2) %i0(state) %i1(gdata)
              sethi 1c3de8, %i1
              or %i1, 488, %i1
         jmp   SOT
              or %g2, 0, %g2
              jmpl [%g0 + %g2]
              nop   
    fefddfe4  [epilogue]
              or %o0, 0, %i0
              jmpl [%i7 + 8]
              restore
              sethi ff1e2000, %g2
              or %g2, 0, %g2
              jmpl [%g0 + %g2]
              nop
(Assignee)

Comment 6

10 years ago
Created attachment 361457 [details] [diff] [review]
patch

This patch passed all the trace-test. Do we need to manually port the patch from tamarin-redux to tracemonkey? I thought this action will be done automatically. There is problem here is about AVMPLUS_xxx_ENDIAN. Since we decide not using NANOJIT_xxx_ENDIAN here and there is no AVMPLUS_xxx_ENDIAN defined in tracemonkey, should we use JS_IS_xxx_ENDIAN here instead?

Comment 7

10 years ago
Sure, we can add the defines as needed. We have our local version of avmplus.h where this can go.

Comment 8

10 years ago
I will see where I can get access to a sparc box and test the patch a bit. Leon, did you fill out the mozilla contributor agreement and all the necessary paperwork to get your patch into the repository?
(Assignee)

Comment 9

10 years ago
(In reply to comment #8)
> I will see where I can get access to a sparc box and test the patch a bit.
> Leon, did you fill out the mozilla contributor agreement and all the necessary
> paperwork to get your patch into the repository?
I remember I have signed the contributor agreement, any other document needed? I already have the check in permission for hg.mozilla.org. Is there other special requirement for tracemonkey?

Comment 10

10 years ago
No, access to hg.mozilla.org is fine.
(Assignee)

Comment 11

10 years ago
Created attachment 361725 [details] [diff] [review]
patch v2

The patch for bug 458060 is landed for tamarin-redux. It's time to put it into tracemonkey and mozilla-central. I passed all the trace-test.js.
Attachment #361725 - Flags: review?(gal)

Updated

10 years ago
Attachment #361725 - Flags: review?(jim)
Attachment #361725 - Flags: review?(gal)
Attachment #361725 - Flags: review+

Comment 12

10 years ago
Comment on attachment 361725 [details] [diff] [review]
patch v2

Patch looks good to me. Its already upstream in tamarin-redux, so we can as well land it now. Afaict this will not affect the browser (yet?), so impact is shell only for now.

Since we don't have infrastructure for sparc, we should try to get a sparc shell build and trace-tests buildbot up and running and linked to our waterfall asap to make sure the code doesn't bitrot.

Soliciting additional review from jim for the configure.in change.

Comment 13

10 years ago
Comment on attachment 361725 [details] [diff] [review]
patch v2

What is the change to line 4338 of js/src/configure.in for?  That looks like a typo.

Would it be possible to merge those two "case $target" statements?  It seems silly to have two.

Updated

10 years ago
Attachment #361725 - Flags: review?(jim) → review-
(Assignee)

Comment 14

10 years ago
(In reply to comment #13)
> (From update of attachment 361725 [details] [diff] [review])
> What is the change to line 4338 of js/src/configure.in for?  That looks like a
> typo.
Wesley write this. If there is no special reason, I believe it is a typo. I already removed this from my patch.
> 
> Would it be possible to merge those two "case $target" statements?  It seems
> silly to have two.
The first one is to define ENABLE_JIT default to 1. The second one is to define AVMPLUS_SPARC only when ENABLE_JIT=1. User can use --disable-jit to turn it off.
(Assignee)

Comment 15

10 years ago
Created attachment 361903 [details] [diff] [review]
patch v3
Attachment #361903 - Flags: review?(jim)

Comment 16

10 years ago
Comment on attachment 361903 [details] [diff] [review]
patch v3

Thanks!
Attachment #361903 - Flags: review?(jim) → review+
(Assignee)

Comment 17

10 years ago
Created attachment 362545 [details] [diff] [review]
Put the flush instruction cashe code to the wrong place during porting.

By the way, do I need another approval or review to integrate it into mozilla-central?
Assignee: general → leon.sha
Status: NEW → ASSIGNED
Attachment #362545 - Flags: review?(gal)

Updated

10 years ago
Attachment #362545 - Flags: review?(gal) → review+

Comment 18

10 years ago
Created attachment 364077 [details] [diff] [review]
patch for 1.9.1 branch

harmless for other platforms
Attachment #364077 - Flags: approval1.9.1?

Comment 19

10 years ago
It was pushed for mozilla-central.
http://hg.mozilla.org/mozilla-central/rev/0e769d46da4b
http://hg.mozilla.org/mozilla-central/rev/678bb683e1c2

Make as fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Attachment #364077 - Flags: approval1.9.1? → approval1.9.1+

Comment 20

10 years ago
Caught a crash with mochitest test_font_face_parser.html

The reason is we should use LDUB/LDUH instead of LDSB/LDSH for LIR_ldcb/LIR_ ldcs.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 21

10 years ago
Created attachment 364493 [details] [diff] [review]
load unsigned rather than load signed
Attachment #364493 - Flags: review?(gal)

Updated

10 years ago
Attachment #364493 - Flags: review?(gal) → review+

Comment 22

10 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/cc8d4e656113
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Comment 23

10 years ago
Pushed:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9865a6c60c5b

The fix for LDUB/LDUH is included.

Updated

10 years ago
Keywords: fixed1.9.1

Comment 24

10 years ago
Land the fix for compileFlatDoubleChar, since it is landed on branch.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ad0391d67e9a

Updated

10 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.