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

WIP: Wasm simd128 support #286

Merged
merged 35 commits into from
Apr 21, 2023
Merged

WIP: Wasm simd128 support #286

merged 35 commits into from
Apr 21, 2023

Conversation

myfreeer
Copy link
Contributor

@myfreeer myfreeer commented Mar 26, 2023

See #282 for previous discussion.

TODOs

  • Inline or rewrite sse2 wrappers borrowed from emscripten

    • _mm_cvtss_f32
    • _mm_movehl_ps
    • _mm_movelh_ps
    • _mm_sqrt_ss
    • _mm_rcp_ps
    • _mm_storeu_ps
    • _mm_store_ss
    • _MM_TRANSPOSE4_PS
  • Guard againist conditions when both sse2 and simd128 enabled by Emscripten (WIP: Wasm simd128 support #286 (comment))

    • affine-mat.h
    • intrin.h
    • mat2.h
    • mat3.h
    • mat4.h
    • quat.h
    • vec4-ext.h
    • vec4.h
  • Change in compile scripts for cross-compiling or a build guide

  • CI-driven tests, cross compile all tests with or without simd128, using appveyor or Github Actions

  • Code style changes

Compiling

Currently the tests can be cross-compiled to wasi using clang and wasi-sdk with cmake arguments below:

-DCMAKE_C_FLAGS="-msimd128"
-DCMAKE_TOOLCHAIN_FILE=/path/to/wasi-sdk-19.0/share/cmake/wasi-sdk.cmake
-DWASI_SDK_PREFIX=/path/to/wasi-sdk-19.0
-DCGLM_USE_TEST=ON

Where /path/to/wasi-sdk-19.0/ is the path to extracted wasi sdk.

The cmake-wasm.yml shows an example for compiling and running tests on github actions.

@kzhsw
Copy link

kzhsw commented Mar 29, 2023

The _mm_storeu_ps here is defined mostly like the build-in wasm_v128_store, and there two would compile to the same result in clang.

static __inline__ void __attribute__((__always_inline__, __nodebug__))
_mm_storeu_ps(float *__p, glmm_128 __a)
{
  struct __unaligned {
    glmm_128 __v;
  } __attribute__((__packed__, __may_alias__));
  ((struct __unaligned *)__p)->__v = __a;
}

static __inline__ void __DEFAULT_FN_ATTRS wasm_v128_store(void *__mem,
                                                          v128_t __a) {
  // UB-free unaligned access copied from xmmintrin.h
  struct __wasm_v128_store_struct {
    __v128_u __v;
  } __attribute__((__packed__, __may_alias__));
  ((struct __wasm_v128_store_struct *)__mem)->__v = __a;
}

@gottfriedleibniz
Copy link

Another potential edge-case that may require handling is compiling w/ Emscripten, e.g., emcc -msimd128 -msse -msse2 ... which will enable both CGLM_SIMD_x86 and CGLM_SIMD_WASM.

@myfreeer
Copy link
Contributor Author

myfreeer commented Apr 2, 2023

Another potential edge-case that may require handling is compiling w/ Emscripten, e.g., emcc -msimd128 -msse -msse2 ... which will enable both CGLM_SIMD_x86 and CGLM_SIMD_WASM.

This could be guarded by some extra check in macros like this, a lot more similar things should be changed.

diff --git a/include/cglm/simd/intrin.h b/include/cglm/simd/intrin.h 
index 17998f5..bf8d119 100644                                        
--- a/include/cglm/simd/intrin.h                                     
+++ b/include/cglm/simd/intrin.h                                     
@@ -113,7 +113,7 @@                                                  
 #  endif
 #endif

-#if defined(CGLM_SIMD_x86) 
+#if defined(CGLM_SIMD_x86) && !defined(CGLM_SIMD_WASM)
 #  include "x86.h"
 #endif

diff --git a/include/cglm/vec4.h b/include/cglm/vec4.h
index d6ee080..e90a3c3 100644
--- a/include/cglm/vec4.h
+++ b/include/cglm/vec4.h 
@@ -181,10 +181,10 @@ glm_vec4_ucopy(vec4 v, vec4 dest) {
 CGLM_INLINE
 void
 glm_vec4_zero(vec4 v) {
-#if defined( __SSE__ ) || defined( __SSE2__ )
-  glmm_store(v, _mm_setzero_ps());
-#elif defined(__wasm__) && defined(__wasm_simd128__)
+#if defined(__wasm__) && defined(__wasm_simd128__)
   glmm_store(v, wasm_f32x4_const_splat(0.f));
+#elif defined( __SSE__ ) || defined( __SSE2__ )
+  glmm_store(v, _mm_setzero_ps());
 #elif defined(CGLM_NEON_FP)
   vst1q_f32(v, vdupq_n_f32(0.0f));
 #else

myfreeer added 2 commits April 2, 2023 12:39
After this, the required options for cmake are listed below:
```
-DCMAKE_C_FLAGS="-msimd128"
-DCMAKE_TOOLCHAIN_FILE=/path/to/wasi-sdk-19.0/share/cmake/wasi-sdk.cmake
-DWASI_SDK_PREFIX=/path/to/wasi-sdk-19.0
-DCGLM_USE_TEST=ON
```
If compiling to wasi with simd128 support, `-DCMAKE_C_FLAGS="-msimd128"` can be removed.
If tests are not needed, `-DCGLM_USE_TEST=ON` can be removed.
@recp
Copy link
Owner

recp commented Apr 7, 2023

@myfreeer great work! any new progress?

@myfreeer
Copy link
Contributor Author

myfreeer commented Apr 8, 2023

@myfreeer great work! any new progress?

Yes, github actions added for tests.

@recp
Copy link
Owner

recp commented Apr 8, 2023

@myfreeer awesome 🚀

@recp
Copy link
Owner

recp commented Apr 20, 2023

@myfreeer any new progress ? It would be nice to merge this asap

@myfreeer
Copy link
Contributor Author

myfreeer commented Apr 21, 2023

@myfreeer any new progress ? It would be nice to merge this asap

@recp
No, I was planning for support of autoconf and meson support, autotools failed to build tests, and meson needs some cross-file with absolute path, some extra helper scripts might be needed for generating this, meson would fail to build tests with maybe the wasm-ld issue. Maybe we should delay supporting those (maybe in another pr), and mark this as ready-for-review.
BTW, which tool is suitable for code style checking?

@recp
Copy link
Owner

recp commented Apr 21, 2023

No, I was planning for support of autoconf and meson support, autotools failed to build tests, and meson needs some cross-file with absolute path, some extra helper scripts might be needed for generating this, meson would fail to build tests with maybe the mesonbuild/meson#10187 issue. Maybe we should delay supporting those (maybe in another pr), and mark this as ready-for-review.

@myfreeer yes this is good approach I think. This PR became huge update, another PR[s] would be awesome, I hope we won't forget it :)

BTW, which tool is suitable for code style checking?

🤷‍♂️ if it will take long time, I or you could do it later

@myfreeer myfreeer marked this pull request as ready for review April 21, 2023 13:38
@recp recp merged commit 4c6fb15 into recp:master Apr 21, 2023
@recp
Copy link
Owner

recp commented Apr 21, 2023

@myfreeer the PR is merged 🚀 many thanks. Feel free to create PRs for additional improvements

@myfreeer
Copy link
Contributor Author

Great! Also, it seems that the github actions ci needs to be manually enabled in this repo to make it work.

@recp
Copy link
Owner

recp commented Apr 22, 2023

I have just enabled the actions 👍

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.

4 participants