Bug 31050 - [Multipatch] JSVALUE32_64 support on ARM
Summary: [Multipatch] JSVALUE32_64 support on ARM
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
Depends on: 31159
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-03 04:04 PST by Zoltan Herczeg
Modified: 2009-11-20 03:35 PST (History)
3 users (show)

See Also:


Attachments
Proposed patch for JSValue32_64 on ARM (22.75 KB, patch)
2009-11-03 04:20 PST, Zoltan Herczeg
barraclough: review-
Details | Formatted Diff | Diff
Second patch (20.62 KB, patch)
2009-11-05 06:03 PST, Zoltan Herczeg
barraclough: review-
Details | Formatted Diff | Diff
Another patch (17.69 KB, patch)
2009-11-06 05:32 PST, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
Never ending story (16.45 KB, patch)
2009-11-09 00:11 PST, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
First patch for JSValue32_64 optimizations (9.63 KB, patch)
2009-11-09 07:23 PST, Zoltan Herczeg
no flags Details | Formatted Diff | Diff
Minor fixes for JSValue32_64 on ARM (4.01 KB, patch)
2009-11-16 03:19 PST, Zoltan Herczeg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Zoltan Herczeg 2009-11-03 04:04:30 PST
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?
Comment 1 Zoltan Herczeg 2009-11-03 04:20:07 PST
Created attachment 42365 [details]
Proposed patch for JSValue32_64 on ARM
Comment 2 Gavin Barraclough 2009-11-04 01:24:49 PST
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.
Comment 3 Zoltan Herczeg 2009-11-05 06:03:49 PST
Created attachment 42566 [details]
Second patch
Comment 4 Gavin Barraclough 2009-11-06 01:59:08 PST
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.
Comment 5 Zoltan Herczeg 2009-11-06 05:32:19 PST
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.
Comment 6 Gavin Barraclough 2009-11-06 22:56:52 PST
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. :-)
Comment 7 Zoltan Herczeg 2009-11-09 00:11:37 PST
Created attachment 42737 [details]
Never ending story
Comment 8 Zoltan Herczeg 2009-11-09 07:23:13 PST
Created attachment 42750 [details]
First patch for JSValue32_64 optimizations
Comment 9 Zoltan Herczeg 2009-11-10 05:20:00 PST
(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
Comment 10 Gavin Barraclough 2009-11-13 14:01:18 PST
Comment on attachment 42737 [details]
Never ending story

Awesome, r+.
Comment 11 Gavin Barraclough 2009-11-13 14:02:58 PST
Comment on attachment 42750 [details]
First patch for JSValue32_64 optimizations

All looks good to me, r+.
Comment 12 Zoltan Horvath 2009-11-13 16:49:49 PST
Comment on attachment 42737 [details]
Never ending story

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

Clearing review flag.
Comment 13 Zoltan Horvath 2009-11-13 17:08:34 PST
Comment on attachment 42750 [details]
First patch for JSValue32_64 optimizations

Landed in 50982.
http://trac.webkit.org/changeset/50982
Clearing review flag.
Comment 14 Zoltan Herczeg 2009-11-16 03:19:20 PST
Created attachment 43285 [details]
Minor fixes for JSValue32_64 on ARM
Comment 15 Zoltan Horvath 2009-11-16 23:18:12 PST
Comment on attachment 43285 [details]
Minor fixes for JSValue32_64 on ARM

Landed in 51067.
http://trac.webkit.org/changeset/51067
Clearing review flag.
Comment 16 Zoltan Horvath 2009-11-20 03:35:20 PST
All reviewed patches have been landed.
Closing bug.