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.
| 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.
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).