Enable fast-interpreter SIMD tests on CI #4125
Conversation
jammar1
requested review from
loganek,
lum1n0us,
no1wudi,
TianlongLiang,
wenyongh,
xujuntwt95329 and
yamt
as code owners
| @@ -97,7 +97,7 @@ jobs: | |||
| "-DWAMR_BUILD_PERF_PROFILING=1", | |||
| "-DWAMR_BUILD_REF_TYPES=1", | |||
| # doesn't support | |||
| # "-DWAMR_BUILD_SIMD=1", | |||
| "-DWAMR_BUILD_SIMD=0", | |||
There was a problem hiding this comment.
I think this was disabled anyway, just making it explicit. When I tried enabling this, SIMDe won't compile because of missing SIMD intrinsics in the SGX toolchain.
/home/runner/work/wasm-micro-runtime/wasm-micro-runtime/product-mini/platforms/linux-sgx/build/_deps/simde-src/simde/wasm/../simde-features.h:319:12: fatal error: emmintrin.h: No such file or directory
319 | #include <emmintrin.h>
| ^~~~~~~~~~~~~
compilation terminated.
| @@ -156,8 +156,8 @@ if (WAMR_BUILD_LIB_RATS EQUAL 1) | |||
| endif () | |||
|
|
|||
| if (WAMR_BUILD_SIMD EQUAL 1 AND WAMR_BUILD_FAST_INTERP EQUAL 1) | |||
| if (NOT (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR WAMR_BUILD_TARGET MATCHES "ARM.*")) | |||
| message(STATUS "SIMDe doesnt support platform " ${WAMR_BUILD_TARGET}) | |||
| if (WAMR_BUILD_PLATFORM STREQUAL "windows") | |||
There was a problem hiding this comment.
I plan to address this later when I can find a Windows computer. Compilation on Visual Studio is failing but this PR strictly improves the test coverage.
There was a problem hiding this comment.
if (NOT (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR WAMR_BUILD_TARGET MATCHES "ARM.*")) does not appear to be equivalent to if (WAMR_BUILD_PLATFORM STREQUAL "windows"). Therefore, we might need to merge the two to ensure compatibility.
There was a problem hiding this comment.
They're not equivalent. I originally removed the check entirely, but I encountered some MSVC build issues.
I'm setting up a Windows computer to take a look at the issue this week, but SIMDe does support various Arm ISAs / x86 / RISC-V so this change is more correct than the existing check.
There was a problem hiding this comment.
Just to clarify, the branch and tests are running and passing on x86 so this check is no longer needed.
There are build issues with Windows in CI (although it builds and runs for me locally), so I changed the check to represent that.
| if (NOT (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR WAMR_BUILD_TARGET MATCHES "ARM.*")) | ||
| message(STATUS "SIMDe doesnt support platform " ${WAMR_BUILD_TARGET}) | ||
| if (WAMR_BUILD_PLATFORM STREQUAL "windows") | ||
| message(STATUS "SIMDe doesnt support platform " ${WAMR_BUILD_PLATFORM}) |
There was a problem hiding this comment.
I wonder if that should be error instead of just a status? At that stage users set WAMR_BUILD_SIMD so they expect to have it; in case it's not supported, maybe we should notify the users more explicitly about it?
|
SIMD tests running successfully on interpreter mode: https://github.com/bytecodealliance/wasm-micro-runtime/actions/runs/13631422473/job/38100056936 |
| @@ -156,8 +156,8 @@ if (WAMR_BUILD_LIB_RATS EQUAL 1) | |||
| endif () | |||
|
|
|||
| if (WAMR_BUILD_SIMD EQUAL 1 AND WAMR_BUILD_FAST_INTERP EQUAL 1) | |||
| if (NOT (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR WAMR_BUILD_TARGET MATCHES "ARM.*")) | |||
| message(STATUS "SIMDe doesnt support platform " ${WAMR_BUILD_TARGET}) | |||
| if (WAMR_BUILD_PLATFORM STREQUAL "windows") | |||
There was a problem hiding this comment.
if (NOT (WAMR_BUILD_TARGET MATCHES "AARCH64.*" OR WAMR_BUILD_TARGET MATCHES "ARM.*")) does not appear to be equivalent to if (WAMR_BUILD_PLATFORM STREQUAL "windows"). Therefore, we might need to merge the two to ensure compatibility.
|
I'm merging it as looks like @lum1n0us 's concern is addressed. |
loganek
merged commit a16c0ae
into
bytecodealliance:dev/simd_for_interp
Enable fast-interpreter tests on CI.
I've kept some platforms disabled as tests were failing (e.g missing SIMD intrinsics headers on SGX, etc).