Bug 31050 – [Multipatch] JSVALUE32_64 support on ARM

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Zoltan Herczeg]

I started this work some weeks ago, but I had to go abroad, and only recently find time to finnish the first patch.

Notes:
  - JSVALUE32_64 is not enabled by default
    - and all optimizations are disabled (as usual for all enhancements).
  - emitPutJITStub... functions are ugly. Somehow stackIndexStart should be propagated from JITStubCall.h. Besides stackIndexStart should be machine dependent constant, defined by the assembler
  - emit_op_div contains x86 code, not looks nice.

And still not found a nice solution for branchDouble with NaN arguments. x86 says anything compared to NaN is <= (set both equal and carry flag). Now branchDouble on ARM has an x86 unordered flag (default true), which just a partial solution, since it sets only the equal flag in case of comparing to NaN. Do you know what ECMA says about double comparison?

[Zoltan Herczeg]

Created attachment 42365 [details]
Proposed patch for JSValue32_64 on ARM

[Gavin Barraclough]

Comment on attachment 42365 [details]
Proposed patch for JSValue32_64 on ARM

As per our conversation on IRC, It will help this change to clean up some of the existing code in svn first, so let's hold this off for:

https://bugs.webkit.org/show_bug.cgi?id=31104

to land first.  I'm going to r- this patch, since these changes will conflict, please resubmit after 31104 is in & you've updated.

ECMA says that math is IEEE754 compliant, and IEEE754 specifies that comparisons between unordered operands (i.e. when either operand is NaN) evaluates to false.  The MacroAssembler interface in ToT is a mess in its handling of unordered operands, I've addressed this in the patch on bug 31104 by making the required handling of unordered operands explicit in the DoubleCondition.

r-, will review a new patch post 31104.

[Zoltan Herczeg]

Created attachment 42566 [details]
Second patch

[Gavin Barraclough]

Comment on attachment 42566 [details]
Second patch

As discussed in irc, I've landed a version of the OBJECT_OFFSETOF(struct JITStackFrame, args) change in r50594.
Also, I've changed the shift behaviour in r50595, so the extra and32's on the shifts should no longer be required.

Also as discussed in irc, some ASSERTs to guard THUNK_RETURN_ADDRESS_OFFSET would be a great improvement.

The empty '#elif PLATFORM(ARM_TRADITIONAL)' in JIT.h was a little confusing, would be great to add a quick comment explaining why this section is empty (something like "// patchOffset... values should go here; JIT optimizations currently not supported on ARM with JSVALUE32_64.").

Otherwise looks good to go, r- since you'll need to update to r50594, r50595, but should be a straight r+ after that.

[Zoltan Herczeg]

Created attachment 42648 [details]
Another patch

The patch contains buildfix for lshift/rsfhift. (rshiftPtr is not exists anymore) Reason: our commiter was away, and on the #irc channel no-one helped us to commit the fix.

[Gavin Barraclough]

Comment on attachment 42648 [details]
Another patch

Hey Zoltan,

I haven't spotted an ASSERT / COMPILE_ASSERT guarding that THUNK_RETURN_ADDRESS_OFFSET  matches the offset of thunkReturnAddress, did that get missed?

Also, could we rename STRINGIZE_HELPER to STRINGIZE, and STRINGIZE to something like STRINGIZE_VALUE, STRINGIZE_VALUE_OF, STRINGIZE_PREPROCESSED.  I think the STRINGIZE macro should match the functionality of the stringize operator (#), in just stringifying the token passed to it, and this is a useful macro in its own right.  The version that doubly nests the macro to extract the value (if it is a preprocessor token) is doing something more than the standard stringize functionality, and I think it needs calling out as such.

Ah, now i see also the comment in JIT.h is also missing - so I'm guessing you just hadn't got around to fixing the last review comments yet. :-)

[Zoltan Herczeg]

Created attachment 42737 [details]
Never ending story

[Zoltan Herczeg]

Created attachment 42750 [details]
First patch for JSValue32_64 optimizations

[Zoltan Herczeg]

(In reply to comment #8)
> Created an attachment (id=42750) [details]
> First patch for JSValue32_64 optimizations

Preliminary results:

SunSpider: 7442.9ms +/- 0.2% (was: 9546 ms, 28% speedup)
V8: 35815.8ms +/- 0.4% (was: 22866 ms, 56% slowdown)
WindScorpion: N/A

[Gavin Barraclough]

Comment on attachment 42737 [details]
Never ending story

Awesome, r+.

[Gavin Barraclough]

Comment on attachment 42750 [details]
First patch for JSValue32_64 optimizations

All looks good to me, r+.

[Zoltan Horvath]

Comment on attachment 42737 [details]
Never ending story

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

Clearing review flag.

[Zoltan Horvath]

Comment on attachment 42750 [details]
First patch for JSValue32_64 optimizations

Landed in 50982.
http://trac.webkit.org/changeset/50982
Clearing review flag.

[Zoltan Herczeg]

Created attachment 43285 [details]
Minor fixes for JSValue32_64 on ARM

[Zoltan Horvath]

Comment on attachment 43285 [details]
Minor fixes for JSValue32_64 on ARM

Landed in 51067.
http://trac.webkit.org/changeset/51067
Clearing review flag.

[Zoltan Horvath]

All reviewed patches have been landed.
Closing bug.