search

boostorg / core

Boost Core Utilities
132 stars 86 forks source link

[bit] Add support for `__builtin_bswap16` #159

Closed Lastique closed 7 months ago

pdimov commented 7 months ago

There's no need for that; all compilers recognize the pattern and turn it into a bswap16 if possible.

Lastique commented 7 months ago

There's no need for that; all compilers recognize the pattern and turn it into a bswap16 if possible.

I'd rather be on the safe side and explicitly say what we want. The intrinsic is there, so why not use it.

pdimov commented 7 months ago

It's not available on all targets and can be non-constexpr, so it's not an automatic "win".

Lastique commented 7 months ago

It's not available on all targets and can be non-constexpr, so it's not an automatic "win".

Do you have an example of that? My understanding is that it is equivalent to bswap32/64, only in case of gcc it appeared a little later than the other two. Clang supported all three all along.

PS: I've updated CI to fix clang failures.

pdimov commented 7 months ago

I'll need to look that up. I'm not sure whether it still applies for the GCC versions we support nowadays.

pdimov commented 7 months ago

Looks like it wasn't supported on GCC 4.7 and Clang 3.1, but this shouldn't be an issue because GCC 4.7 doesn't have __has_builtin so it's not going to get used anyway. (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52624)

Appears to be constexpr when supported too.

Lastique commented 7 months ago

Right, that's why I'm enabling it only since gcc 4.8.