Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wasm] Turn Wasm exception handling on by default and use WasmEnableExceptio… #84573

Merged
merged 3 commits into from
Apr 15, 2023

Conversation

lewing
Copy link
Member

@lewing lewing commented Apr 10, 2023

…nHandling as the default value for WasmEnableSIMD

…nHandling as the default value for WasmEnableSIMD
@lewing lewing requested a review from radical as a code owner April 10, 2023 16:39
@ghost ghost assigned lewing Apr 10, 2023
@lewing lewing requested a review from radekdoulik April 10, 2023 16:40
@lewing lewing changed the title Turn Wasm exception handling on by default and use WasmEnableExceptio… [wasm] Turn Wasm exception handling on by default and use WasmEnableExceptio… Apr 10, 2023
@radical
Copy link
Member

radical commented Apr 11, 2023

WBT/node tests failing with:

Application arguments: --run async_main_with_args_Release_False.dll abc foobar
MONO_WASM: instantiate_wasm_module() failed: {}
MONO_WASM: Stacktrace:

CompileError: WebAssembly.instantiate(): unexpected section <Tag> (enable with --experimental-wasm-eh) @+15097
CompileError: WebAssembly.instantiate(): unexpected section <Tag> (enable with --experimental-wasm-eh) @+15097
exiting due to exception: CompileError: WebAssembly.instantiate(): unexpected section <Tag> (enable with --experimental-wasm-eh) @+15097

@lewing
Copy link
Member Author

lewing commented Apr 11, 2023

WBT/node tests failing with:

Application arguments: --run async_main_with_args_Release_False.dll abc foobar
MONO_WASM: instantiate_wasm_module() failed: {}
MONO_WASM: Stacktrace:

CompileError: WebAssembly.instantiate(): unexpected section <Tag> (enable with --experimental-wasm-eh) @+15097
CompileError: WebAssembly.instantiate(): unexpected section <Tag> (enable with --experimental-wasm-eh) @+15097
exiting due to exception: CompileError: WebAssembly.instantiate(): unexpected section <Tag> (enable with --experimental-wasm-eh) @+15097

yeah we need to enable the feature for all the host runtimes

<WasmEnableSIMD Condition="'$(WasmEnableSIMD)' == ''">true</WasmEnableSIMD>
<!-- Post Wasm MVP features -->
<WasmEnableExceptionHandling Condition="'$(WasmEnableExceptionHandling)' == ''">true</WasmEnableExceptionHandling>
<WasmEnableSIMD Condition="'$(WasmEnableSIMD)' == ''">$(WasmEnableExceptionHandling)</WasmEnableSIMD>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious: why to base the default SIMD value on EH value?

Copy link
Member Author

@lewing lewing Apr 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it is much less of an improvement without EH and EH is more broadly compatible/deployed than SIMD so if you are explicitly disabling EH you probably want to disable SIMD too, whereas if you were disable SIMD for compat reasons you may not need to disable EH. That was my thought at least

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EH is more broadly compatible/deployed than SIMD

only in Safari. Chrome, Firefox and nodejs have SIMD support longer than EH. (https://webassembly.org/roadmap/)

I think the new behavior is a bit confusing. Maybe we could introduce a new property, like WasmEnableAdvancedFeatures or WasmEnableNextGen to switch the defaults of SIMD, EH and other non 1.1 features. What do you think?

@kg
Copy link
Member

kg commented Apr 14, 2023

Looks fine other than radek's comment

@lewing
Copy link
Member Author

lewing commented Apr 14, 2023

@radekdoulik should we change the benchmarks to explicitly set these settings based on their name/intension?

@lewing lewing merged commit b9483f3 into dotnet:main Apr 15, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants