Bug 233444 - [WASM-Function-References] Simplify representation of Wasm Type
Summary: [WASM-Function-References] Simplify representation of Wasm Type
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dmitry
URL:
Keywords: InRadar
Depends on:
Blocks: 247393
  Show dependency treegraph
 
Reported: 2021-11-23 03:40 PST by Dmitry
Modified: 2022-11-02 17:08 PDT (History)
9 users (show)

See Also:


Attachments
Patch (27.09 KB, patch)
2021-11-23 03:43 PST, Dmitry
no flags Details | Formatted Diff | Diff
Patch (28.69 KB, patch)
2021-11-23 22:33 PST, Dmitry
no flags Details | Formatted Diff | Diff
Patch (28.90 KB, patch)
2022-02-13 23:26 PST, Dmitry
no flags Details | Formatted Diff | Diff
Patch (36.34 KB, patch)
2022-02-16 04:04 PST, Dmitry
no flags Details | Formatted Diff | Diff
Patch (42.54 KB, patch)
2022-02-20 00:03 PST, Dmitry
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry 2021-11-23 03:40:54 PST
Wasm Type has weird ballast called signature index, actually there is no such field in the wasm spec - https://github.com/WebAssembly/function-references/blob/master/proposals/function-references/Overview.md.
Signature index was introduced to implement `call_ref` instruction because there wasn't an ability to get Signature from the obtained type.
In this bug this ability is introduced and we can simplify Type to the form <kind, nullability, typeIndex>, where typeIndex is an index in Wasm Type section.
This will simplify subsequent work for new Wasm types like struct or arrays.
Comment 1 Dmitry 2021-11-23 03:43:36 PST
Created attachment 445023 [details]
Patch
Comment 2 Dmitry 2021-11-23 22:33:55 PST
Created attachment 445066 [details]
Patch
Comment 3 Yusuke Suzuki 2021-11-25 09:37:39 PST
Comment on attachment 445066 [details]
Patch

Acn you relove SignatureIndex completely? Currently, it is globally registered, but it should use per module type index onstead. And if we start using that in one place, then we should use it in all the place.
Comment 4 Yusuke Suzuki 2021-11-25 09:39:21 PST
(In reply to Yusuke Suzuki from comment #3)
> Comment on attachment 445066 [details]
> Patch
> 
> Acn you relove SignatureIndex completely? Currently, it is globally
> registered, but it should use per module type index onstead. And if we start
> using that in one place, then we should use it in all the place.

Oops, typos. “Can you remove SignatureIndex completely?”
Comment 5 Radar WebKit Bug Importer 2021-11-30 03:41:20 PST
<rdar://problem/85859071>
Comment 6 Yusuke Suzuki 2022-02-05 11:17:01 PST
Comment on attachment 445066 [details]
Patch

OK, I think it is fine for now.
Comment 7 EWS 2022-02-05 11:18:14 PST
Tools/Scripts/svn-apply failed to apply attachment 445066 [details] to trunk.
Please resolve the conflicts and upload a new patch.
Comment 8 Dmitry 2022-02-13 23:26:33 PST
Created attachment 451864 [details]
Patch
Comment 9 Yusuke Suzuki 2022-02-16 03:00:23 PST
Comment on attachment 451864 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:10
> +        Replace Signature index with index into Wasm Type section.
> +        Add corresponding mapping to be able to get itroduced type index
> +        via given function index.

Can you describe the motivation of this change in the ChangeLog? Why do we need to use this instead of current SignatureIndex?

> Source/JavaScriptCore/wasm/WasmFormat.h:260
> +    uint32_t typeIndex;

We should use more explicit name since it is not well-typed.
Let's name it like, typeIndexInModule etc.

> Source/JavaScriptCore/wasm/generateWasmOpsHeader.py:224
> +    unsigned index;

Ditto. Change this to typeIndexInModule.
Comment 10 Dmitry 2022-02-16 04:04:38 PST
Created attachment 452175 [details]
Patch
Comment 11 Dmitry 2022-02-16 04:05:30 PST
Hi, I've updated the changelog to describe more motivation of this change and fixed naming issues
Comment 12 Yusuke Suzuki 2022-02-16 11:40:16 PST
Comment on attachment 452175 [details]
Patch

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

> Source/JavaScriptCore/wasm/WasmGlobal.cpp:107
> +                Wasm::SignatureIndex paramIndex = m_type.typeIndexInModule;

This looks incorrect since typeIndexInModule is not SignatureIndex.

> Source/JavaScriptCore/wasm/js/JSWebAssemblyHelpers.h:194
> +                Wasm::SignatureIndex paramIndex = type.typeIndexInModule;

This looks incorrect since typeIndexInModule is not SignatureIndex.

> Source/JavaScriptCore/wasm/js/WebAssemblyFunction.cpp:282
> +                    slowPath.append(jit.branchPtr(CCallHelpers::NotEqual, scratchGPR, CCallHelpers::TrustedImmPtr(type.typeIndexInModule)));

This looks incorrect since typeIndexInModule is not SignatureIndex.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:273
> +                                Wasm::SignatureIndex paramIndex = global.type.typeIndexInModule;

This looks incorrect since typeIndexInModule is not SignatureIndex.

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:327
> +                                Wasm::SignatureIndex paramIndex = global.type.typeIndexInModule;

This looks incorrect since typeIndexInModule is not SignatureIndex.
Comment 13 Dmitry 2022-02-20 00:03:32 PST
Created attachment 452682 [details]
Patch