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

bswap.h: fix GCC requirements for bswap* builtins #4201

Merged
merged 1 commit into from Feb 24, 2024

Conversation

rilysh
Copy link
Contributor

@rilysh rilysh commented Feb 24, 2024

Summary

  1. __builtin_bswap{32,64} were added in GCC 4.3, and __builtin_bswap16 was added in GCC 4.8, however, currently, the GCC requirements in bswap.h file has >= 10. This requirement of GCC version is false for bswap* but true for __has_builtin() (as it first was added in GCC 10.1). As bswap* builtins were added before GCC 10, the preprocessor check will always going to be true for bswap but will be false if GCC version is < 10 as __has_builtin() won't be present. Since the byteswap function, on x86-64, can boil down to a single bswap instruction, this optimization may left behind (unless GCC do some pattern matching). To avoid this, just use the compiler macros (for GCC: _GNUC_, clang: _GNUC_ or _clang_) and if the compiler is neither GCC or Clang, fall-back to native implementation.

  2. Remove the useless casts (uint{16,32,64}_t) from the constants. These constants already has their own suffix, and casting to a different type will just get ignored as the return value already gets casts to it's appropriate type.

  3. Previously, Clang couldn't able to use __builtin_bswap* (even if it was newer) as LLVM define GNUC macro to a specific constant (usually lower than GCC's (GNUC) and on my system it's 4) which is indeed < 10. The first comment also fixes this issue.

Link: https://gcc.gnu.org/onlinedocs/gcc-4.3.0/gcc/Other-Builtins.html
Link: https://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Other-Builtins.html
Link: https://libc-alpha.sourceware.narkive.com/PfaB4BGP/patch-byteswap-h-fix-gcc-ver-test-for-builtin-bswap32

Checklist

References

Provide links to datasheets or other documentation that helped you implement this pull request.

1. __builtin_bswap{32,64} were added in GCC 4.3, and __builtin_bswap16
was added in GCC 4.8, however, currently, the GCC requirements in
bswap.h file has >= 10. This requirement of GCC version is false for
bswap* but true for __has_builtin() (as it first was added in GCC 10.1).
As bswap* builtins were added before GCC 10, the preprocessor check will
always going to be true for bswap but will be false if GCC version is
< 10 as __has_builtin() won't be present. Since the byteswap function,
on x86-64, can boil down to a single bswap instruction, this optimization
may left behind (unless GCC do some pattern matching). To avoid this,
just use the compiler macros (for GCC: __GNUC__, clang: __GNUC__ or
__clang__) and if the compiler is neither GCC or Clang, fall-back to
native implementation.

2. Remove the useless casts (uint{16,32,64}_t) from the constants. These
constants already has their own suffix, and casting to a different type
will just get ignored as the return value already gets casts to it's
appropriate type.

3. Previously, Clang couldn't able to use __builtin_bswap* (even if it was
newer) as LLVM define __GNUC__ macro to a specific constant (usually lower
than GCC's (__GNUC__) and on my system it's 4) which is indeed < 10. The
first comment also fixes this issue.

Link: <https://gcc.gnu.org/onlinedocs/gcc-4.3.0/gcc/Other-Builtins.html>
Link: <https://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Other-Builtins.html>
Link: <https://libc-alpha.sourceware.narkive.com/PfaB4BGP/patch-byteswap-h-fix-gcc-ver-test-for-builtin-bswap32>
@OBattler OBattler merged commit cd255ba into 86Box:master Feb 24, 2024
51 of 52 checks passed
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

Successfully merging this pull request may close these issues.

None yet

2 participants