Bug 233444 – [WASM-Function-References] Simplify representation of Wasm Type

• Status: NEW
• Component: WebAssembly

[Dmitry]

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.

[Dmitry]

Created attachment 445023 [details]
Patch

[Dmitry]

Created attachment 445066 [details]
Patch

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

(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?”

[Radar WebKit Bug Importer]

<rdar://problem/85859071>

[Yusuke Suzuki]

Comment on attachment 445066 [details]
Patch

OK, I think it is fine for now.

[EWS]

Tools/Scripts/svn-apply failed to apply attachment 445066 [details] to trunk.
Please resolve the conflicts and upload a new patch.

[Dmitry]

Created attachment 451864 [details]
Patch

[Yusuke Suzuki]

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.

[Dmitry]

Created attachment 452175 [details]
Patch

[Dmitry]

Hi, I've updated the changelog to describe more motivation of this change and fixed naming issues

[Yusuke Suzuki]

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.

[Dmitry]

Created attachment 452682 [details]
Patch