bswap.h: fix GCC requirements for bswap* builtins#4201
Merged
Conversation
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
__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.
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.
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.