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

std/crc64 doesn't build for 32-bit x86 #145

Closed
nigeltao opened this issue Apr 6, 2024 · 1 comment
Closed

std/crc64 doesn't build for 32-bit x86 #145

nigeltao opened this issue Apr 6, 2024 · 1 comment

Comments

@nigeltao
Copy link
Collaborator

nigeltao commented Apr 6, 2024

std/xz depends on std/crc64, which recently changed:

  • f32bfe9 "std/crc64: optimize for x86+SSE4.2"

These new SSE4.2 intrinsics build fine on 64-bit x86 but not with the -m32 flag, because _mm_cvtsi64_si128 and _mm_extract_epi64 are apparently unsupported on 32-bit x86. Some GCC discussion:


$ gcc      -DWUFFS_CONFIG__FUZZLIB_MAIN fuzz/c/std/xz_fuzzer.c -o /dev/null


$ gcc -m32 -DWUFFS_CONFIG__FUZZLIB_MAIN fuzz/c/std/xz_fuzzer.c -o /dev/null
In file included from fuzz/c/std/xz_fuzzer.c:71:
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c: In function ‘wuffs_crc64__ecma_hasher__up_x86_sse42’:
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c:36935:10: warning: implicit declaration of function ‘_mm_cvtsi64_si128’; did you mean ‘_mm_cvtsi32_si128’? [-Wimplicit-function-declaration]
36935 |   v_s0 = _mm_cvtsi64_si128((int64_t)(v_s));
      |          ^~~~~~~~~~~~~~~~~
      |          _mm_cvtsi32_si128
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c:36935:10: error: incompatible types when assigning to type ‘__m128i’ from type ‘int’
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c:36964:21: warning: implicit declaration of function ‘_mm_extract_epi64’; did you mean ‘_mm_extract_si64’? [-Wimplicit-function-declaration]
36964 |   v_s = ((uint64_t)(_mm_extract_epi64(_mm_xor_si128(v_aa, _mm_xor_si128(v_w2, _mm_slli_si128(v_w1, (int32_t)(8u)))), (int32_t)(1u))));
      |                     ^~~~~~~~~~~~~~~~~
      |                     _mm_extract_si64


$ clang      -DWUFFS_CONFIG__FUZZLIB_MAIN fuzz/c/std/xz_fuzzer.c -o /dev/null


$ clang -m32 -DWUFFS_CONFIG__FUZZLIB_MAIN fuzz/c/std/xz_fuzzer.c -o /dev/null
In file included from fuzz/c/std/xz_fuzzer.c:71:
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c:36935:10: warning: implicit declaration of function '_mm_cvtsi64_si128' is invalid in C99 [-Wimplicit-function-declaration]
  v_s0 = _mm_cvtsi64_si128((int64_t)(v_s));
         ^
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c:36935:8: error: assigning to '__m128i' (vector of 2 'long long' values) from incompatible type 'int'
  v_s0 = _mm_cvtsi64_si128((int64_t)(v_s));
       ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
fuzz/c/std/../../../release/c/wuffs-unsupported-snapshot.c:36964:21: warning: implicit declaration of function '_mm_extract_epi64' is invalid in C99 [-Wimplicit-function-declaration]
  v_s = ((uint64_t)(_mm_extract_epi64(_mm_xor_si128(v_aa, _mm_xor_si128(v_w2, _mm_slli_si128(v_w1, (int32_t)(8u)))), (int32_t)(1u))));
                    ^
2 warnings and 1 error generated.
@nigeltao
Copy link
Collaborator Author

nigeltao commented Apr 6, 2024

Build failure was picked up by oss-fuzz:

https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=67858

nigeltao added a commit that referenced this issue Apr 6, 2024
nigeltao added a commit that referenced this issue Jul 9, 2024
If I understand the #148
discussion correctly, a user overrode WUFFS_BASE__CPU_ARCH__X86_FAMILY
(without also providing /arch:AVX) to enable SSE4.2 code (even though,
technically, x86_64 doesn't guarantee SSE4.2, only SSE2). This seemed to
work fine in practice, but was unsupported by Wuffs and 'broke' by
fixing #145, now looking for WUFFS_BASE__CPU_ARCH__X86_64 instead of
WUFFS_BASE__CPU_ARCH__X86_FAMILY.

WUFFS_BASE__CPU_ARCH__X86_FAMILY has since been renamed to
WUFFS_PRIVATE_IMPL__CPU_ARCH__X86_FAMILY and then removed entirely.

As SSE4.2 (roughly equivalent to x86-64-v2) seems to work fine at
compile time (with the existing cpuid detection at runtime), enable it
by default for MSVC, without need /arch.

Enabling x86-64-v3 too, by default, is held back for now, pending
further confirmation from MSVC users. The #148 user was exercising
Wuffs' PNG codec, which uses SSE4.2 but not AVX or AVX2. Currently, only
Wuffs' JPEG codec (and YCbCr conversion) uses AVX or AVX2.

GCC and Clang don't need this fiddliness, because they support
"__attribute__((target(arg)))".

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

No branches or pull requests

1 participant