Bug 192983 – [JSC] Reenable baseline JIT on mips

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Guillaume Emont]

Since the new bytecode format, we compile with the CLoop back end on mips. As a first step to reenable the JIT on MIPS, we should enable LLInt with the mips backend of the offline assembler.

[Guillaume Emont]

Created attachment 357953 [details]
Patch

WIP Patch for initial discussion, might not be ready for review yet, especially as I will be offline next week.

[Guillaume Emont]

I think the change in bmalloc and the new test deserve a comment.
While testing this llint patch, I found that we would occasionally trigger a RELEASE_ASSERT in ChakraCore/test/JSON/jx1.js. I worked on a smaller test case that would trigger the issue more reliably, and that is the new stress/json-parse-big-object.js.

The issue seems to be a concurrency issue, but I have not yet found what is happening exactly, but we found empirically (thanks Dominik!) that initializing g_gigacageBasePtrs seems to fix the issue.

This is the stack trace I get when the RELEASE_ASSERT triggers:

Thread 1 "jsc" received signal SIGABRT, Aborted.
0x74047d60 in raise () from /home/guijemont/dev/metrological/buildroot-jsc/output.ci20/staging/lib/libc.so.6
(gdb) bt
#0  0x74047d60 in raise () from /home/guijemont/dev/metrological/buildroot-jsc/output.ci20/staging/lib/libc.so.6
#1  0x740496b8 in abort () from /home/guijemont/dev/metrological/buildroot-jsc/output.ci20/staging/lib/libc.so.6
#2  0x77d305d4 in Gigacage::free (kind=Gigacage::JSValue, p=0x737def80) at ../../Source/WTF/wtf/Gigacage.cpp:100
#3  0x0047e4c8 in JSC::MarkedArgumentBuffer::~MarkedArgumentBuffer (this=0x7fff5670, __in_chrg=<optimized out>) at ../../Source/JavaScriptCore/runtime/ArgList.h:59
#4  0x7791f6b8 in JSC::Stringifier::~Stringifier (this=0x7fff5628, __in_chrg=<optimized out>) at ../../Source/JavaScriptCore/runtime/JSONObject.cpp:83
#5  0x778d6b58 in JSC::JSONProtoFuncStringify (exec=0x7fff5848) at ../../Source/JavaScriptCore/runtime/JSONObject.cpp:840
#6  0x772aa9dc in llint_op_debug_wide () at /home/guijemont/dev/toolchains/mips32el/buildroot_2018.02.2/mipsel-buildroot-linux-gnu/include/c++/6.4.0/cmath:650
Backtrace stopped: frame did not save the PC


In that context, it is worth noting the following:
(gdb) p ((Gigacage::BasePtrs*)g_gigacageBasePtrs)->jsValue
$1 = (void *) 0x0

Which goes in the direction of the above RELEASE_ASSERT to be impossible to trigger... Though I guess that the scavenger thread might have been able to somehow reset the value before SIGABRT?

At the time of SIGABRT, the scavenger thread is waiting at Scavenger.cpp:385

[Dominik Inführ]

Comment on attachment 357953 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357953&action=review

> Source/JavaScriptCore/jit/GPRInfo.h:736
> +    static const unsigned numberOfRegisters = 12;

I think we shouldn't change this for now, since this is only needed for DFG and we didn't change that on ARM either.

> Source/JavaScriptCore/jit/GPRInfo.h:770
> +        static const GPRReg registerForIndex[numberOfRegisters] = { regT0, regT1, regT2, regT3, regT4, regT5, regT6, regT7, regT8, regT9, regT10, regCS0 };

ditto

> Source/JavaScriptCore/jit/GPRInfo.h:788
> +            11, InvalidIndex, InvalidIndex, InvalidIndex, InvalidIndex, InvalidIndex, InvalidIndex, InvalidIndex,

ditto

[Guillaume Emont]

Created attachment 358312 [details]
Patch

Rebased and addressed Dominik's comment. I think the patch is ready for review from other people. Particularly interested in thoughts on the g_gigacageBasePtrs initialisation. I ran tests on a ci20 board with this patch and got 0 failures.

[Guillaume Emont]

(In reply to Guillaume Emont from comment #4)
> Created attachment 358312 [details]
> Patch
> 
> Rebased and addressed Dominik's comment. I think the patch is ready for
> review from other people. Particularly interested in thoughts on the
> g_gigacageBasePtrs initialisation. I ran tests on a ci20 board with this
> patch and got 0 failures.

Oops, remove that statement about 0 failures, I was accidentally compiling with cloop, need to run tests again.

[Guillaume Emont]

(In reply to Guillaume Emont from comment #5)
> (In reply to Guillaume Emont from comment #4)
> > Created attachment 358312 [details]
> > Patch
> > 
> > Rebased and addressed Dominik's comment. I think the patch is ready for
> > review from other people. Particularly interested in thoughts on the
> > g_gigacageBasePtrs initialisation. I ran tests on a ci20 board with this
> > patch and got 0 failures.
> 
> Oops, remove that statement about 0 failures, I was accidentally compiling
> with cloop, need to run tests again.

I have now rerun the tests on the ci20 with a correct compilation (using the mips backend to the offlineassembler), and now I can tell I got 0 failures :).

[Guillaume Emont]

Created attachment 358503 [details]
Patch

New version that also enables baseline JIT since it just works (0 test failure on ci20).

[Guillaume Emont]

Created attachment 358798 [details]
Patch

New version rebased after arm/DFG landed, which means I had to change a bit how Platform.h is updated. Also skip stress/new-largeish-contiguous-array-with-size.js.

[Guillaume Emont]

Since the DFG/arm patch got rolled out, this patch likely won't apply any more, I think the best is to wait until DFG/arm relands.

[Guillaume Emont]

(In reply to Guillaume Emont from comment #9)
> Since the DFG/arm patch got rolled out, this patch likely won't apply any
> more, I think the best is to wait until DFG/arm relands.

Note that the DFG/arm patch re-landed permanently, and this patch should apply. All it needs is a review.

[Guillaume Emont]

Created attachment 359728 [details]
Patch

Rebased the patch, and removed changes to Gigacage, as they are not needed any more since r240175. Anyone for a review before it has to be rebased again?

[Mark Lam]

Comment on attachment 359728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=359728&action=review

LGTM but I'm not a MIPS expert.  I'm ok with landing this if the JSC tests passes on MIPS (not sure of that's what the jsc-mips EWS bot tests).  It'd be better if someone else who know MIPS better also give this a review.

> Source/JavaScriptCore/offlineasm/mips.rb:884
> +        when "loadbs", "loadbsp"
>              $asm.puts "lb #{mipsFlippedOperands(operands)}"

This is only valid if MIPS is 32-bit only.  loadbs should load and sign extend up to 32-bit.  On 64-bit, loadbsp should load and sign extend up to 64-bit.

[Guillaume Emont]

(In reply to Mark Lam from comment #12)
> Comment on attachment 359728 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=359728&action=review
> 
> LGTM but I'm not a MIPS expert.  I'm ok with landing this if the JSC tests
> passes on MIPS (not sure of that's what the jsc-mips EWS bot tests).  It'd
> be better if someone else who know MIPS better also give this a review.

Dominik knows some MIPS and has had a look. I'm not sure we have any reviewer that really knows MIPS (maybe Yusuke?).
Currently the jsc-mips EWS is only building, but I did run the tests manually. We have a buildbot that runs tests though.
We do have a plan to set up test devices for the MIPS EWS (we actually already have the devices), but we're waiting for DFG to be enabled again so that the tests can run fast enough.

> 
> > Source/JavaScriptCore/offlineasm/mips.rb:884
> > +        when "loadbs", "loadbsp"
> >              $asm.puts "lb #{mipsFlippedOperands(operands)}"
> 
> This is only valid if MIPS is 32-bit only.  loadbs should load and sign
> extend up to 32-bit.  On 64-bit, loadbsp should load and sign extend up to
> 64-bit.

This might be a bit too implicit, and could be the subject of another patch, but we currently only support and test mips 32 bits.

[WebKit Commit Bot]

Comment on attachment 359728 [details]
Patch

Clearing flags on attachment: 359728

Committed r240432: <https://trac.webkit.org/changeset/240432>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/47511597>