Bug 192983 - [JSC] Reenable baseline JIT on mips
Summary: [JSC] Reenable baseline JIT on mips
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Other Linux
: P2 Normal
Assignee: Guillaume Emont
URL:
Keywords: InRadar
Depends on: 192496
Blocks: 191163
  Show dependency treegraph
 
Reported: 2018-12-21 09:30 PST by Guillaume Emont
Modified: 2019-01-24 04:05 PST (History)
6 users (show)

See Also:


Attachments
Patch (29.94 KB, patch)
2018-12-21 09:46 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (28.61 KB, patch)
2019-01-04 02:06 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (28.72 KB, patch)
2019-01-07 09:56 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (29.33 KB, patch)
2019-01-10 08:52 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff
Patch (27.82 KB, patch)
2019-01-22 03:13 PST, Guillaume Emont
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Guillaume Emont 2018-12-21 09:30:43 PST
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.
Comment 1 Guillaume Emont 2018-12-21 09:46:24 PST
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.
Comment 2 Guillaume Emont 2018-12-21 10:10:38 PST
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
Comment 3 Dominik Inführ 2018-12-21 10:20:36 PST
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
Comment 4 Guillaume Emont 2019-01-04 02:06:47 PST
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.
Comment 5 Guillaume Emont 2019-01-04 08:50:27 PST
(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.
Comment 6 Guillaume Emont 2019-01-05 02:38:29 PST
(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 :).
Comment 7 Guillaume Emont 2019-01-07 09:56:10 PST
Created attachment 358503 [details]
Patch

New version that also enables baseline JIT since it just works (0 test failure on ci20).
Comment 8 Guillaume Emont 2019-01-10 08:52:52 PST
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.
Comment 9 Guillaume Emont 2019-01-11 02:18:56 PST
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.
Comment 10 Guillaume Emont 2019-01-16 02:29:33 PST
(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.
Comment 11 Guillaume Emont 2019-01-22 03:13:49 PST
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?
Comment 12 Mark Lam 2019-01-23 12:01:39 PST
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.
Comment 13 Guillaume Emont 2019-01-24 03:29:50 PST
(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.
Comment 14 WebKit Commit Bot 2019-01-24 04:04:42 PST
Comment on attachment 359728 [details]
Patch

Clearing flags on attachment: 359728

Committed r240432: <https://trac.webkit.org/changeset/240432>
Comment 15 WebKit Commit Bot 2019-01-24 04:04:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-01-24 04:05:32 PST
<rdar://problem/47511597>