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

SSE: Check !defined(__EMSCRIPTEN__) where intrinsics are unavailable on WASM #12917

Merged
merged 3 commits into from
Apr 7, 2024

Conversation

fwcd
Copy link
Member

@fwcd fwcd commented Mar 4, 2024

Some SSE intrinsics are unavailable in Emscripten, in particular _MM_SET_DENORMALS_ZERO_MODE1, therefore we have to disable them when targeting Emscripten/WASM.

Footnotes

  1. As per https://emscripten.org/docs/porting/simd.html#id8,

@@ -445,7 +445,7 @@ void SoundDeviceNetwork::callbackProcessClkRef() {
// This disables the denormals calculations, to avoid a
// performance penalty of ~20
// https://github.com/mixxxdj/mixxx/issues/7747
#ifdef __SSE__
#if defined(__SSE__) && !defined(__EMSCRIPTEN__)
Copy link
Member

Choose a reason for hiding this comment

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

Here are some details:
https://emscripten.org/docs/porting/simd.html

It sounds like denormals to zero mode IS available. Can you check that and adjust the debug message accordingly?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, it's available but since the denormals mode is fixed, we cannot use the set functions, so there is not really a point in having these blocks execute. The flush zero mode is fixed too, but in this case to being off, which isn't the mode we want. _mm_setcsr isn't available either, so we can't use the fallbacks from denormalsarezero.h.

Can you check that and adjust the debug message accordingly?

Do mean emitting a qDebug() message when targeting __EMSCRIPTEN__? Or only cond-compiling out the actual _MM_SET invocations and emitting a debug message where the fixed mode doesn't match our desired mode?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, than just add a comment about the situation. My original concern was that this is a implementation defined thing.
The background is, that denormals are so slow, that the EQs and other filters are not usable. I am curious if they are usable with emscripten.

Copy link
Member Author

@fwcd fwcd Mar 8, 2024

Choose a reason for hiding this comment

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

Haven't gotten around to test EQ and filters in Mixxx yet (there are still some more foundational issues to be resolved, including audio support, before we can actually test that), but came across this interesting discussion upstream: WebAssembly/design#148

Looks like flush-to-zero (FTZ)/denormals-as-zero (DAZ) were deliberately avoided in favor of IEEE-compliance, though they've hinted at adding it as an opt-in.

Edit: Mentioned here: https://github.com/WebAssembly/design/blob/main/FutureFeatures.md#flushing-subnormal-values-to-zero

@fwcd
Copy link
Member Author

fwcd commented Mar 20, 2024

This part from the porting guide confuses me a bit:

Always returns _MM_DENORMALS_ZERO_ON. I.e. denormals are available.

_MM_DENORMALS_ZERO_ON would mean denormals-as-zero are available, but the second sentence stating that denormals are available would suggest that this mode is off (which would be consistent with the Wasm design discussions).

Anyway, I've updated the comments to mention that these modes are non-configurable when targeting Emscripten/Wasm.

@fwcd fwcd requested a review from daschuer April 7, 2024 18:24
@daschuer
Copy link
Member

daschuer commented Apr 7, 2024

We have a test that checks for denormals here:

qWarning() << "Network Sound: Denormals to zero mode is not working. "

Did you see the warning?

#ifdef __SSE__

// On Emscripten (WebAssembly) denormals-as-zero/flush-as-zero is not
// configurable, for discussion and links see
Copy link
Member

Choose a reason for hiding this comment

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

Please add here a comment about the current situation. Do we suffer from denormals calculations or not?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can probably say that these are not supported, but to assess the actual performance, we'd need probably need audio, which isn't ready yet. Since it's not configurable, there is probably not much point in discussing this as a performance consideration until Emscripten/Wasm add a way to configure it.

@fwcd
Copy link
Member Author

fwcd commented Apr 7, 2024

Did you see the warning?

Yes (the build deployed at https://fwcd.dev/m1xxx displays it):

warning [0x1dbc25c0] Network Sound: Denormals to zero mode is not working. EQs and effects may suffer high CPU load

Though we'll probably first be able to properly test whether this is indeed a performance issue when the Web Audio backend is ready (which, unfortunately, seems to be a non-trivial roadblock due to issues with Asyncify, see PortAudio/portaudio#887 and fwcd/m1xxx#69).

@daschuer
Copy link
Member

daschuer commented Apr 7, 2024

Calculation denormals is done on the CPU and it takes significant lower independent which programming model is used. So we can assume that this is a real issue until we have proved the opposite. Can you please adjust the comment accordingly and than we can merge.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Thank you very much. LGTM

@daschuer daschuer merged commit 0416ba6 into mixxxdj:main Apr 7, 2024
10 checks passed
@fwcd fwcd deleted the sse-wasm-fixes branch April 7, 2024 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants