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

Emscripten-wasm: Add options to enable SIMD and Threads #12

Closed
wants to merge 1 commit into from

Conversation

Squareys
Copy link
Contributor

Hi @mosra !

As these feature are getting ready/are ready in modern browsers, here's a proposal how they could be supported in the toolchain.

Best,
Jonathan

@mosra mosra added this to the 2020.0b milestone Oct 21, 2020
@mosra
Copy link
Owner

mosra commented Oct 21, 2020

Eh, not sure about this at all. The main problem is that the toolchain file gets read only once, so in case you'd ever want to enable/disable these features later, changing the flag will not do anything (and you won't know it did nothing) unless you recreate the build dir.

Second, this is something that I think should not get enabled on a global level. Even now in Magnum I have a bunch of tests and utility executables where I want to have different flags on each to test different functionality. And if a certain app (let's say a computationally-heavy example) needs to enable threads and/or SIMD, it shouldn't get enabled for all others, making all others suddenly not run on browsers w/o threads/SIMD.

The way threads are currently done in CMake is in the FindThreads module. So I'd say the "canonical" way to enable this for Emscripten is to either fork that module so it does the right thing when you find_package(Threads) on Emscripten, or do something in the toolchain file that makes the FindThreads module think this is a "classic Unix-y system" where pthread works. Or maybe it "just works" already when using that module? Did you try?

For the SIMD stuff, I'm not sure. I'm still investigating general SIMD optimizations in magnum, so I don't know which way is the right way and whether e.g. a wasm module with SIMD ops can run on non-SIMD-enabled browsers if you never hit the SIMD code path, or if you need a totally separate binary. I'd say you just add these flags in your global CMAKE_CXX_FLAGS instead for now.

@Squareys
Copy link
Contributor Author

Squareys commented Oct 22, 2020

@mosra Thing with Threads is that it needs to be enabled in every compiled object, not just in the final application. To use threads, I need to recompile every single dependency of my application, so on a global level is mandatory here.

For SIMD I'm not sure, especially since that only requires a compile-time flag, that could be enabled per compiled object.
I read something about SIMD potentially being bw-compatible or something (not sure how that would work or if that's true), but I will most likely detect threads/simd at runtime and download the right WASM depending on what is available.

I think there is a argument in "WASM-SIMD" or "WASM-Threads" being their own platforms and therefor this is just a nicer way of saying -DCMAKE_TOOLCHAIN_FILE=../toolchains/generic/Emscripten-wasm-threads.cmake. Might look ugly, but consider that (at least for threads), everything needs to be recompiled anyway (like switching to Android toolchain), so this sets expectations correctly.

@mosra
Copy link
Owner

mosra commented Oct 22, 2020

I honestly don't think neither suggested option is the right direction. This doesn't belong to a toolchain file, it's just two non-mandatory compiler flags.

  1. Doing -DEMSCRIPTEN_TARGET_THREADS=ON instead of -DCMAKE_CXX_FLAGS=-fpthread doesn't solve anything, it only obscures a widely-known compiler flag under something that's specific to this one particular toolchain file. Which only means everyone who sees it has to search this variable in some (nonexistent) documentation and waste more time.
  2. It's not a completely new platform, it's just two compiler flags. Adding dedicated toolchain files would only mean I'd have to maintain four (or more) copies of the toolchain file instead of two (which is already painful enough).

Seriously, what's wrong with modifying CMAKE_CXX_FLAGS that we need such a solution?

@Squareys
Copy link
Contributor Author

@mosra I did not seriously propose the second option, it overlaps very much with Emscripten-wasm.cmake so as you said that would just be hard to maintain.

It's not just a compiler flag, -pthread is a compiler and linker flag that needs to be enabled everywhere. (For SIMD, that's a different discussion, that's just a compile flag).

What I'm saying that libraries/objects that have vs do not have this flag cannot be linked together.

I guess I can go for -DCMAKE_CXX_FLAGS=-pthreads -DCMAKE_EXE_LINKER_FLAGS=-pthreads for all projects, though, yes.

@mosra mosra added the scrapped label Oct 23, 2020
@mosra
Copy link
Owner

mosra commented Oct 23, 2020

a compiler and linker flag

According to my experience, CMake passes compiler flags to the linker as well ;) Maybe it's platform-dependent, so check to be really sure.

What I'm saying that libraries/objects that have vs do not have this flag cannot be linked together.

The presence of the flag in the toolchain file doesn't make that any easier tho. If you accidentally build a dependency without the toolchain option enabled, it's the same as if you'd forget the flag.

Closing this -- simply use -DCMAKE_BLA_FLAGS=bla for that.

@mosra mosra closed this Oct 23, 2020
@Squareys
Copy link
Contributor Author

it's the same as if you'd forget the flag.

True 👍

I solved this now by setting CFLAGS, CXXFLAGS and LDFlAGS environment variables depending on the target. Those are consumed by CMAKE to initialize the mentioned flags. That avoids accidentally forgetting to add the flag when compiling multiple projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants