Bug 189498

Summary: [WebAssembly] Move type conversion code of JSToWasm return type to JS wasm wrapper
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: ews-watchlist, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch sbarati: review+

Description Yusuke Suzuki 2018-09-11 05:00:05 PDT
[WebAssembly] Move type conversion code of JSToWasm return type to JS wasm wrapper
Comment 1 Yusuke Suzuki 2018-09-11 05:03:10 PDT
Created attachment 349390 [details]
Patch
Comment 2 Yusuke Suzuki 2018-09-11 05:06:34 PDT
Created attachment 349391 [details]
Patch
Comment 3 Yusuke Suzuki 2018-09-11 05:56:56 PDT
Eventually I would like to merge these code into wrapper side.
Comment 4 Yusuke Suzuki 2018-09-26 01:38:46 PDT
Ping?
Comment 5 Saam Barati 2018-09-30 11:46:50 PDT
Comment on attachment 349391 [details]
Patch

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

r=me

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:164
> +    return rawResult;

Is there any value in keeping the above code in some form for a debug assert?
Comment 6 Saam Barati 2018-09-30 11:48:26 PDT
Comment on attachment 349391 [details]
Patch

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

> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:223
> +        auto isNaN = jit.branchDouble(CCallHelpers::DoubleNotEqualOrUnordered, FPRInfo::returnValueFPR, FPRInfo::returnValueFPR);

We should really just add something in AssemblyHelpers sometime in the future for branchIfNaN
Comment 7 Yusuke Suzuki 2018-10-01 02:24:04 PDT
Comment on attachment 349391 [details]
Patch

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

>> Source/JavaScriptCore/wasm/js/JSToWasm.cpp:223
>> +        auto isNaN = jit.branchDouble(CCallHelpers::DoubleNotEqualOrUnordered, FPRInfo::returnValueFPR, FPRInfo::returnValueFPR);
> 
> We should really just add something in AssemblyHelpers sometime in the future for branchIfNaN

Sounds fine. I'll add this in a separate patch.

>> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:164
>> +    return rawResult;
> 
> Is there any value in keeping the above code in some form for a debug assert?

Eventually, I would like to remove this entire function to merge this functionality into the generated code.
So I think we should add assertions in the generated code instead of this function.
Comment 8 Yusuke Suzuki 2018-10-01 02:25:46 PDT
Committed r236651: <https://trac.webkit.org/changeset/236651>
Comment 9 Radar WebKit Bug Importer 2018-10-01 02:26:28 PDT
<rdar://problem/44904397>