Bug 148945

Summary: Implement the arithmetic instructions for doubles in WebAssembly
Product: WebKit Reporter: Sukolsak Sakshuwong <sukolsak>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, keith_miller, sukolsak
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 148913    
Bug Blocks: 146064    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for ARM 1
none
Patch for ARM 2
none
Update to ToT
none
Remove maxFrameExtentForSlowPathCall
none
Patch none

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.