Bug 189498 - [WebAssembly] Move type conversion code of JSToWasm return type to JS wasm wrapper
Summary: [WebAssembly] Move type conversion code of JSToWasm return type to JS wasm wr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-09-11 05:00 PDT by Yusuke Suzuki
Modified: 2018-10-01 02:26 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.07 KB, patch)
2018-09-11 05:03 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (6.05 KB, patch)
2018-09-11 05:06 PDT, Yusuke Suzuki
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>