Bug 148945 - Implement the arithmetic instructions for doubles in WebAssembly
Summary: Implement the arithmetic instructions for doubles in WebAssembly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 148913
Blocks: 146064
  Show dependency treegraph
 
Reported: 2015-09-07 14:43 PDT by Sukolsak Sakshuwong
Modified: 2015-09-18 17:31 PDT (History)
5 users (show)

See Also:


Attachments
Patch (28.37 KB, patch)
2015-09-07 15:36 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (29.08 KB, patch)
2015-09-08 20:47 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (29.21 KB, patch)
2015-09-09 02:34 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch (18.69 KB, patch)
2015-09-11 01:06 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch for ARM 1 (17.31 KB, patch)
2015-09-17 21:19 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Patch for ARM 2 (17.64 KB, patch)
2015-09-17 21:19 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Update to ToT (18.28 KB, patch)
2015-09-18 16:15 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff
Remove maxFrameExtentForSlowPathCall (17.73 KB, text/plain)
2015-09-18 16:25 PDT, Sukolsak Sakshuwong
no flags Details
Patch (17.73 KB, patch)
2015-09-18 16:26 PDT, Sukolsak Sakshuwong
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sukolsak Sakshuwong 2015-09-07 14:43:54 PDT
Implement the arithmetic instructions for doubles (float64) in WebAssembly.
Comment 1 Sukolsak Sakshuwong 2015-09-07 15:36:03 PDT
Created attachment 260749 [details]
Patch
Comment 2 Sukolsak Sakshuwong 2015-09-08 20:47:28 PDT
Created attachment 260828 [details]
Patch
Comment 3 Sukolsak Sakshuwong 2015-09-09 02:34:42 PDT
Created attachment 260851 [details]
Patch
Comment 4 Sukolsak Sakshuwong 2015-09-11 01:06:00 PDT
Created attachment 260994 [details]
Patch
Comment 5 Geoffrey Garen 2015-09-11 10:52:47 PDT
Comment on attachment 260994 [details]
Patch

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

> Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:77
> +// On Windows we need to wrap fmod; on other platforms we can call it directly.
> +// On ARMv7 we assert that all function pointers have to low bit set (point to thumb code).
> +#if CALLING_CONVENTION_IS_STDCALL || CPU(ARM_THUMB2)
> +static double JIT_OPERATION operationFmod(double x, double y)

to low bit => the low bit

I don't understand this comment. Why do we need to wrap fmod on Windows? Your comment says that we need to wrap fmod on Windows and not other platforms, but your code tests for all stdcall and/or thumb2 platforms. Why is it relevant that ARMv7 code is thumb code?
Comment 6 Sukolsak Sakshuwong 2015-09-17 21:19:20 PDT
Created attachment 261480 [details]
Patch for ARM 1
Comment 7 Sukolsak Sakshuwong 2015-09-17 21:19:43 PDT
Created attachment 261481 [details]
Patch for ARM 2
Comment 8 Sukolsak Sakshuwong 2015-09-18 14:03:01 PDT
Comment on attachment 261480 [details]
Patch for ARM 1

Michael Saboff has tested this patch on both ARMv7 and ARM64. This patch doesn't use a wrapper for fmod. It passes every WASM tests.
Comment 9 Sukolsak Sakshuwong 2015-09-18 16:15:08 PDT
Created attachment 261541 [details]
Update to ToT
Comment 10 Sukolsak Sakshuwong 2015-09-18 16:20:44 PDT
(In reply to comment #5)
> Comment on attachment 260994 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=260994&action=review
> 
> > Source/JavaScriptCore/wasm/WASMFunctionCompiler.h:77
> > +// On Windows we need to wrap fmod; on other platforms we can call it directly.
> > +// On ARMv7 we assert that all function pointers have to low bit set (point to thumb code).
> > +#if CALLING_CONVENTION_IS_STDCALL || CPU(ARM_THUMB2)
> > +static double JIT_OPERATION operationFmod(double x, double y)
> 
> to low bit => the low bit
> 
> I don't understand this comment. Why do we need to wrap fmod on Windows?
> Your comment says that we need to wrap fmod on Windows and not other
> platforms, but your code tests for all stdcall and/or thumb2 platforms. Why
> is it relevant that ARMv7 code is thumb code?

I copied this code from dfg/DFGSpeculativeJIT.cpp. My guess is that fmod was written in non-thumb code. I have removed the wrapper for fmod and tested this patch on ARMv7 and ARM64 with the help of Michael. It passes every WASM test.
Comment 11 Sukolsak Sakshuwong 2015-09-18 16:25:40 PDT
Created attachment 261542 [details]
Remove maxFrameExtentForSlowPathCall
Comment 12 Sukolsak Sakshuwong 2015-09-18 16:26:39 PDT
Created attachment 261543 [details]
Patch
Comment 13 Geoffrey Garen 2015-09-18 16:44:27 PDT
Comment on attachment 261543 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2015-09-18 17:31:21 PDT
Comment on attachment 261543 [details]
Patch

Clearing flags on attachment: 261543

Committed r190002: <http://trac.webkit.org/changeset/190002>
Comment 15 WebKit Commit Bot 2015-09-18 17:31:26 PDT
All reviewed patches have been landed.  Closing bug.