Bug 79609 – LLInt should support JSVALUE64

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 128933 [details]
it begins

Work in progress. Note that this includes https://bugs.webkit.org/show_bug.cgi?id=79608 for now, since that hasn't landed yet.

[Filip Pizlo]

Created attachment 128941 [details]
work in progress

[Filip Pizlo]

Created attachment 129143 [details]
work in progress

Filled in more of the 64-bit code.

[Filip Pizlo]

Created attachment 129358 [details]
work in progress

Almost half way there!

[Filip Pizlo]

Created attachment 130527 [details]
work in progress

More than half done.

[Filip Pizlo]

Created attachment 130528 [details]
work in progress

Same as previous but with typo fixes.  Turns out the offlineasm's parser and semantic analyzer emits useful error messages.

[Filip Pizlo]

Created attachment 130957 [details]
work in progress

Almost there.

[Filip Pizlo]

<rdar://problem/10063437>

[Filip Pizlo]

Created attachment 130966 [details]
work in progress

All of the code is written.  Offlineasm compiles it fine, but I have no idea if clang will be happy with offlineasm's input, and if it is, if it'll run at all.

Next step: having fun playing with seg faults.

[Filip Pizlo]

Created attachment 131132 [details]
work in progress

It's passing most run-javascriptcore-tests.

Still more work to be done, though.

[Filip Pizlo]

Created attachment 131136 [details]
work in progress

Fixed some silly goofs, like one that was preventing the get_by_id/put_by_id fast paths from being taken.

[Filip Pizlo]

Created attachment 131158 [details]
the patch

It's ready.

[Filip Pizlo]

Created attachment 131159 [details]
the patch

Wrote a changelog.

[Build Bot]

Comment on attachment 131159 [details]
the patch

Attachment 131159 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/11933072

[Filip Pizlo]

Created attachment 131161 [details]
the patch

Fix Mac build and a minor bug in op_to_jsnumber.

[Oliver Hunt]

Comment on attachment 131161 [details]
the patch

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

This looks fine to me (other than the unnecessary release mode hash lookups :( ) but i'll let gavin make the final call

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:233
> +    if (exec->globalData().interpreter->getOpcodeID(pc[0].u.opcode) == op_ret) {

Doesn't this result in a hash lookup even in release builds without logging?

[Filip Pizlo]

(In reply to comment #16)
> (From update of attachment 131161 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131161&action=review
> 
> This looks fine to me (other than the unnecessary release mode hash lookups :( ) but i'll let gavin make the final call

This patch does not introduce any unnecessary hash lookups. 

> 
> > Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:233
> > +    if (exec->globalData().interpreter->getOpcodeID(pc[0].u.opcode) == op_ret) {
> 
> Doesn't this result in a hash lookup even in release builds without logging?

No. That function only gets called with execution tracing is enabled. It is not enabled by default in any builds.

[Gavin Barraclough]

Comment on attachment 131161 [details]
the patch

r+, but see email

[Gavin Barraclough]

Comment on attachment 131161 [details]
the patch

Scratch the email comment.  r+, all looks good, commit away!

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/110383

[Ryosuke Niwa]

I'm not certain but maybe this regressed Dromaeo/sunspider-crypto-sha1 (or that undone the temporary gain made by http://trac.webkit.org/changeset/109705/)?

http://webkit-perf.appspot.com/graph.html#tests=[[39655,2001,32196]]&sel=none&displayrange=7&datatype=running