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.

Sign up for GitHub

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
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b82a427
SSE: Check !defined(__EMSCRIPTEN__) where unavailable
fwcd Mar 4, 2024
637e34e
SoundDevice: Update comments on DAZ/FTZ for Wasm
fwcd Mar 20, 2024
8c62564
SoundDevice: Clarify that DAZ/FTZ are unsupported on Wasm
fwcd Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/soundio/sounddevicenetwork.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,10 +442,16 @@ void SoundDeviceNetwork::callbackProcessClkRef() {

if (!m_denormals) {
m_denormals = true;

// This disables the denormals calculations, to avoid a
// performance penalty of ~20
// https://github.com/mixxxdj/mixxx/issues/7747
#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.

// https://github.com/mixxxdj/mixxx/pull/12917

#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 •
edited
Loading

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

if (!_MM_GET_DENORMALS_ZERO_MODE()) {
qDebug() << "SSE: Enabling denormals to zero mode";
_MM_SET_DENORMALS_ZERO_MODE(_MM_DENORMALS_ZERO_ON);
Expand Down
7 changes: 6 additions & 1 deletion src/soundio/sounddeviceportaudio.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -906,10 +906,15 @@ int SoundDevicePortAudio::callbackProcessClkRef(
#endif
m_bSetThreadPriority = true;

#ifdef __SSE__
// This disables the denormals calculations, to avoid a
// performance penalty of ~20
// https://github.com/mixxxdj/mixxx/issues/7747

// On Emscripten (WebAssembly) denormals-as-zero/flush-as-zero is not
// configurable, for discussion and links see
// https://github.com/mixxxdj/mixxx/pull/12917

#if defined(__SSE__) && !defined(__EMSCRIPTEN__)
if (!_MM_GET_DENORMALS_ZERO_MODE()) {
qDebug() << "SSE: Enabling denormals to zero mode";
_MM_SET_DENORMALS_ZERO_MODE(_MM_DENORMALS_ZERO_ON);
Expand Down
2 changes: 1 addition & 1 deletion src/util/denormalsarezero.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
#define _MM_DENORMALS_ZERO_OFF 0x0000
#endif

#ifdef __SSE__
#if defined(__SSE__) && !defined(__EMSCRIPTEN__)

#include <xmmintrin.h>

Expand Down