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

HACL SIMD support incorrectly detected under WASI #126458

Closed
brettcannon opened this issue Nov 5, 2024 · 5 comments
Closed

HACL SIMD support incorrectly detected under WASI #126458

brettcannon opened this issue Nov 5, 2024 · 5 comments
Labels
build The build process and cross-build OS-wasi type-bug An unexpected behavior, bug, or error

Comments

@brettcannon
Copy link
Member

brettcannon commented Nov 5, 2024

Bug report

Bug description:

I'm getting a lot of:

/opt/wasi-sdk/bin/clang -c -I../../Modules/_hacl -I../../Modules/_hacl/include -D_BSD_SOURCE -D_DEFAULT_SOURCE -fno-strict-overflow -Wsign-compare -Wunreachable-code -DNDEBUG -g -O3 -Wall -Wno-unused-value -Wno-empty-body -Qunused-arguments -Wno-parentheses-equality -Wno-unused-value -Wno-empty-body -Qunused-arguments -Wno-parentheses-equality  -std=c11 -Wextra -Wno-unused-parameter -Wno-int-conversion -Wno-missing-field-initializers -Wstrict-prototypes -Werror=implicit-function-declaration -fvisibility=hidden  -I../../Include/internal -I../../Include/internal/mimalloc -IObjects -IInclude -IPython -I. -I../../Include     -msimd128 -msse -msse2 -msse3 -msse4.1 -msse4.2 -DHACL_CAN_COMPILE_VEC128 -o Modules/_hacl/Hacl_Hash_Blake2s_Simd128.o ../../Modules/_hacl/Hacl_Hash_Blake2s_Simd128.c
In file included from ../../Modules/_hacl/Hacl_Hash_Blake2s_Simd128.c:26:
In file included from ../../Modules/_hacl/internal/Hacl_Hash_Blake2s_Simd128.h:40:
../../Modules/_hacl/internal/../Hacl_Hash_Blake2s_Simd128.h:55:3: error: unknown type name 'Lib_IntVector_Intrinsics_vec128'

That seems to be defined at:

typedef __m128i Lib_IntVector_Intrinsics_vec128;

But that whole header file is guarded by:

#if defined(__x86_64__) || defined(_M_X64) || defined(__i386__) || defined(_M_IX86)

Unfortunately, if you add defined(__wasi__) to that guard you then get:

/opt/wasi-sdk-24.0-x86_64-linux/lib/clang/18/include/immintrin.h:14:2: error: "This header is only meant to be used on x86 and x64 architecture"
   14 | #error "This header is only meant to be used on x86 and x64 architecture"
      |  ^

To me that suggests the check at:

cpython/configure.ac

Lines 7854 to 7906 in 5e91684

# The SIMD files use aligned_alloc, which is not available on older versions of
# Android.
if test "$ac_sys_system" != "Linux-android" || test "$ANDROID_API_LEVEL" -ge 28; then
dnl This can be extended here to detect e.g. Power8, which HACL* should also support.
AX_CHECK_COMPILE_FLAG([-msse -msse2 -msse3 -msse4.1 -msse4.2],[
[LIBHACL_SIMD128_FLAGS="-msse -msse2 -msse3 -msse4.1 -msse4.2"]
AC_DEFINE([HACL_CAN_COMPILE_SIMD128], [1], [HACL* library can compile SIMD128 implementations])
# macOS universal2 builds *support* the -msse etc flags because they're
# available on x86_64. However, performance of the HACL SIMD128 implementation
# isn't great, so it's disabled on ARM64.
AC_MSG_CHECKING([for HACL* SIMD128 implementation])
if test "$UNIVERSAL_ARCHS" == "universal2"; then
[LIBHACL_SIMD128_OBJS="Modules/_hacl/Hacl_Hash_Blake2s_Simd128_universal2.o"]
AC_MSG_RESULT([universal2])
else
[LIBHACL_SIMD128_OBJS="Modules/_hacl/Hacl_Hash_Blake2s_Simd128.o"]
AC_MSG_RESULT([standard])
fi
], [], [-Werror])
fi
AC_SUBST([LIBHACL_SIMD128_FLAGS])
AC_SUBST([LIBHACL_SIMD128_OBJS])
# The SIMD files use aligned_alloc, which is not available on older versions of
# Android.
#
# Although AVX support is not guaranteed on Android
# (https://developer.android.com/ndk/guides/abis#86-64), this is safe because we do a
# runtime CPUID check.
if test "$ac_sys_system" != "Linux-android" || test "$ANDROID_API_LEVEL" -ge 28; then
AX_CHECK_COMPILE_FLAG([-mavx2],[
[LIBHACL_SIMD256_FLAGS="-mavx2"]
AC_DEFINE([HACL_CAN_COMPILE_SIMD256], [1], [HACL* library can compile SIMD256 implementations])
# macOS universal2 builds *support* the -mavx2 compiler flag because it's
# available on x86_64; but the HACL SIMD256 build then fails because the
# implementation requires symbols that aren't available on ARM64. Use a
# wrapped implementation if we're building for universal2.
AC_MSG_CHECKING([for HACL* SIMD256 implementation])
if test "$UNIVERSAL_ARCHS" == "universal2"; then
[LIBHACL_SIMD256_OBJS="Modules/_hacl/Hacl_Hash_Blake2b_Simd256_universal2.o"]
AC_MSG_RESULT([universal2])
else
[LIBHACL_SIMD256_OBJS="Modules/_hacl/Hacl_Hash_Blake2b_Simd256.o"]
AC_MSG_RESULT([standard])
fi
], [], [-Werror])
fi
AC_SUBST([LIBHACL_SIMD256_FLAGS])
AC_SUBST([LIBHACL_SIMD256_OBJS])

is too broad since the header files don't work on non-x86 architectures.

This probably requires either a CPU/platform check before checking the flag support or an explicit opt-out for at least WASI.

(And I have no idea why CI isn't running into this problem while I am locally; bad configure caching via

- name: "Restore host config.cache"
uses: actions/cache@v4
with:
path: ${{ env.CROSS_BUILD_WASI }}/config.cache
# Should be kept in sync with the other config.cache step above.
key: ${{ github.job }}-${{ runner.os }}-${{ env.IMAGE_VERSION }}-${{ env.WASI_SDK_VERSION }}-${{ env.WASMTIME_VERSION }}-${{ inputs.config_hash }}-${{ hashFiles('Tools/wasm/wasi.py') }}-${{ env.pythonLocation }}
?)

CPython versions tested on:

CPython main branch

Operating systems tested on:

Other

Linked PRs

@brettcannon brettcannon added build The build process and cross-build OS-wasi type-bug An unexpected behavior, bug, or error labels Nov 5, 2024
@brettcannon
Copy link
Member Author

@erlend-aasland how would you prefer to structure this in configure.ac?

@erlend-aasland
Copy link
Contributor

@erlend-aasland how would you prefer to structure this in configure.ac?

I think @picnixz already started hashing out better SIMD checks in #125022.

@picnixz
Copy link
Contributor

picnixz commented Nov 6, 2024

This probably requires either a CPU/platform check before checking the flag support or an explicit opt-out for at least WASI.

The easiest way to patch it for now is to opt-out I think and just use the normal headers. That way, we won't have any issue.

I think @picnixz already started hashing out better SIMD checks

My checks only detect the SIMD compiler flags separately instead in one go so it could not necessarily be a solution. It does not care about the architecture itself and just to see whether the flags are supported or not. Concerning the headers that can or cannot be included, they should be done at compile time so we cannot rely on CPUID checks. But what we can do is create a .h which contains some #define telling me whether I can or cannot include that or this header (this would help a lot).

Maybe @corona10 has some ideas on this topic? we could perhaps discuss it on the Discord (I've created a thread for #125022, so you could join (or I can just @ you)).

@corona10
Copy link
Member

corona10 commented Nov 6, 2024

I will try to follow up the issue at this weekend :)

@brettcannon
Copy link
Member Author

I have a PR up to at least opt out for now. If someone could give the two-liner (plus comments) a quick check I would appreciate it so I can get unblocked.

brettcannon added a commit that referenced this issue Nov 6, 2024
Requires an extra `-msimd128` flag and the `*mmintrin.h` header files are exclusive to x86-family CPUs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build The build process and cross-build OS-wasi type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants