476042 - Integrate sparc nanojit intro tracemonkey

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wesley W. Garland]

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

Comment 1

[Wesley W. Garland]

Attachment: Initial patch to add tracing on sparc

Comment 2

[Wesley W. Garland]

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.

Comment 3

[Andreas Gal :gal]

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!

Comment 4

[Leon Sha]

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.

Comment 5

[Wesley W. Garland]

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

Comment 6

[Leon Sha]

Attachment: 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

[Andreas Gal :gal]

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

Comment 8

[Andreas Gal :gal]

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?

Comment 9

[Leon Sha]

(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

[Andreas Gal :gal]

No, access to hg.mozilla.org is fine.

Comment 11

[Leon Sha]

Attachment: 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.

Comment 12

[Andreas Gal :gal]

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

[Jim Blandy :jimb]

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.

Comment 14

[Leon Sha]

(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.

Comment 15

[Leon Sha]

Attachment: patch v3

Comment 16

[Jim Blandy :jimb]

Comment on attachment 361903 [details] [diff] [review]
patch v3

Thanks!

Comment 17

[Leon Sha]

Attachment: 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?

Comment 18

[Ginn Chen]

Attachment: patch for 1.9.1 branch

harmless for other platforms

Comment 19

[Ginn Chen]

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.

Comment 20

[Ginn Chen]

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.

Comment 21

[Ginn Chen]

Attachment: load unsigned rather than load signed

Comment 22

[Ginn Chen]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/cc8d4e656113

Comment 23

[Ginn Chen]

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

The fix for LDUB/LDUH is included.

Comment 24

[Ginn Chen]

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

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