RFR: JDK-8300783: Consolidate byteswap implementations [v13]

Kim Barrett kbarrett at openjdk.org
Wed Feb 15 12:21:07 UTC 2023 

On Tue, 31 Jan 2023 20:05:21 GMT, Justin King <jcking at openjdk.org> wrote:

>> Deduplicate byte swapping implementations by consolidating them into `utilities/byteswap.hpp`, following `std::byteswap` introduced in C++23. Further simplification of `Bytes` will follow in https://github.com/openjdk/jdk/pull/12078.
>
> Justin King has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix copyright
>   
>   Signed-off-by: Justin King <jcking at google.com>

Changes requested by kbarrett (Reviewer).

src/hotspot/cpu/aarch64/bytes_aarch64.hpp line 54:

> 52:   static inline void put_Java_u2(address p, u2 x)     { put_native_u2(p, byteswap<u2>(x)); }
> 53:   static inline void put_Java_u4(address p, u4 x)     { put_native_u4(p, byteswap<u4>(x)); }
> 54:   static inline void put_Java_u8(address p, u8 x)     { put_native_u8(p, byteswap<u8>(x)); }

Explicit type parameters for byteswap aren't needed or desirable; let the type be deduced.  This is a
common theme throughout, so I didn't comment on all occurrences.

src/hotspot/share/jfr/utilities/jfrBigEndian.hpp line 40:

> 38: # define bigendian_16(x) byteswap<u2>(x)
> 39: # define bigendian_32(x) byteswap<u4>(x)
> 40: # define bigendian_64(x) byteswap<u8>(x)

The explicit template parameters might really be needed, unlike (nearly?) every other call.

src/hotspot/share/utilities/byteswap.hpp line 36:

> 34: 
> 35: #include "metaprogramming/enableIf.hpp"
> 36: #include "utilities/debug.hpp"

I think debug.hpp is only being included for STATIC_ASSERT?  We have static_assert now, which likely provides
better error messages.

src/hotspot/share/utilities/byteswap.hpp line 38:

> 36: #include "utilities/debug.hpp"
> 37: #include "utilities/globalDefinitions.hpp"
> 38: #include "utilities/macros.hpp"

Not sure why macros.hpp is being directly included.  I don't see anything from it that is being used here.

src/hotspot/share/utilities/byteswap.hpp line 54:

> 52: struct ByteswapImpl;
> 53: 
> 54: template <typename T, ENABLE_IF(CanByteswapImpl<T>::value)>

I don't think we need CanByteswapImpl here.  Just use `std::is_integral<T>::value` and let the sized specializations of ByteswapImpl determine what sizes are available.  Yes, one might get a better error
message this way if passing in some extended integer value, but that's not something I think we need
to worry about.

src/hotspot/share/utilities/byteswap.hpp line 55:

> 53: 
> 54: template <typename T, ENABLE_IF(CanByteswapImpl<T>::value)>
> 55: ALWAYSINLINE T byteswap(T x) {

Can this be constexpr?  I think it probably can, assuming all the various intrinsics are constexpr.

Also, we don't generally use ALWAYSINLINE unless there is a demonstrated reason to do so.  There are some 
operator new's that use it to help with the allocation recording.  The oop_oop_iterate suite uses it because
they were found to often not inline, and ensuring they were inlined was found to be performance critical.
Other than that, there are a handful of uses, some of which might not actually be needed.

src/hotspot/share/utilities/byteswap.hpp line 55:

> 53: 
> 54: template <typename T, ENABLE_IF(CanByteswapImpl<T>::value)>
> 55: ALWAYSINLINE T byteswap(T x) {

There should be a description comment for this function.

src/hotspot/share/utilities/byteswap.hpp line 57:

> 55: ALWAYSINLINE T byteswap(T x) {
> 56:   using U = std::make_unsigned_t<T>;
> 57:   STATIC_ASSERT(sizeof(T) == sizeof(U));

I don't think this static assert adds anything other than clutter.

src/hotspot/share/utilities/byteswap.hpp line 70:

> 68: // We support 8-bit integer types to be compatible with C++23's std::byteswap.
> 69: template <typename T>
> 70: struct ByteswapFallbackImpl<T, 1> {

I think we don't need this specialization, nor do we need the copied size==1 specializations for ByteswapImpl.
Just have one unconditional specialization of ByteswapImpl<T, 1>.

src/hotspot/share/utilities/byteswap.hpp line 72:

> 70: struct ByteswapFallbackImpl<T, 1> {
> 71:   STATIC_ASSERT(CanByteswapImpl<T>::value);
> 72:   STATIC_ASSERT(sizeof(T) == 1);

(Throughout) I don't think these static asserts add anything other than clutter.

src/hotspot/share/utilities/byteswap.hpp line 138:

> 136: 
> 137: template <typename T>
> 138: struct ByteswapImpl<T, 2> final {

(Throughout) I don't think `final` adds any value here.

src/hotspot/share/utilities/byteswap.hpp line 143:

> 141: 
> 142:   ALWAYSINLINE T operator()(T x) const {
> 143:     return static_cast<T>(__builtin_bswap16(static_cast<uint16_t>(x)));

(Throughout) I think we can just replace T with uint16_t / whatever, and dispense with the casts.

src/hotspot/share/utilities/byteswap.hpp line 176:

> 174: // architecture-specific byteswap instruction, if available. If it is not available, GCC emits the
> 175: // exact same implementation that underpins its __builtin_bswap in libgcc as there is really only
> 176: // one way to implement it, as we have in fallback.

Well that's kind of annoying.  Or alternatively, it's annoying that we can't use the portable version
everywhere and depend on compilers to reliably recognize the pattern and replace with an
architecture-specific implementation if available.  Oh well.  Good that you noticed this.

src/hotspot/share/utilities/byteswap.hpp line 243:

> 241: #elif defined(TARGET_COMPILER_xlc)
> 242: 
> 243: #include <builtins.h>

Not needed if we (can) use clang `__builtin_bswapN`?

src/hotspot/share/utilities/byteswap.hpp line 264:

> 262:   ALWAYSINLINE T operator()(T x) const {
> 263:     unsigned short y;
> 264:     __store2r(static_cast<unsigned short>(x), &y);

Can we not use `__builtin_bswapN` for these, since the compiler is clang-based?

src/hotspot/share/utilities/byteswap.hpp line 282:

> 280: };
> 281: 
> 282: #if defined(_ARCH_PWR7) && defined(_ARCH_PPC64)

Does the need for this conditionalization go away if we use `__builtin_bswap64`?

src/hotspot/share/utilities/copy.cpp line 131:

> 129: 
> 130:       if (swap) {
> 131:         tmp = byteswap<T>(tmp);

I think the explicit template argument shouldn't be needed - just let it be deduced from `tmp`.

src/hotspot/share/utilities/moveBits.hpp line 31:

> 29: #include "metaprogramming/enableIf.hpp"
> 30: #include "utilities/byteswap.hpp"
> 31: #include "utilities/debug.hpp"

I think debug.hpp is only being included for STATIC_ASSERT?  We have static_assert now, which
likely provides better error messages.

src/hotspot/share/utilities/moveBits.hpp line 51:

> 49: struct ReverseBitsImpl;
> 50: 
> 51: template <typename T, ENABLE_IF(CanReverseBitsImpl<T>::value)>

Similarly to byteswap, I don't think we need CanReverseBitsImpl, just std::is_integral and let the supported
sizes be determined from the provided specializations.

src/hotspot/share/utilities/moveBits.hpp line 52:

> 50: 
> 51: template <typename T, ENABLE_IF(CanReverseBitsImpl<T>::value)>
> 52: ALWAYSINLINE T reverse_bits(T x) {

So far as I know, ALWAYSINLINE does not imply constexpr.  So this is a feature regression.

src/hotspot/share/utilities/moveBits.hpp line 53:

> 51: template <typename T, ENABLE_IF(CanReverseBitsImpl<T>::value)>
> 52: ALWAYSINLINE T reverse_bits(T x) {
> 53:   return ReverseBitsImpl<T>{}(x);

I think I would have preferred to have the change to bit reversal by separate from byteswap changes.
With these changes, I also think this file should be renamed to reverse_bits.hpp or something like that.
It was called moveBits because it contained both bit and byte swapping.

src/hotspot/share/utilities/moveBits.hpp line 81:

> 79:     x = ((x & rep_3333) << 2) | ((x >> 2) & rep_3333);
> 80:     x = ((x & rep_0F0F) << 4) | ((x >> 4) & rep_0F0F);
> 81:     return byteswap<T>(static_cast<T>(x));

If we happen to use this on a platform that has hardware support for bitswap, and the implementation
of byteswap is an intrinsic, it seems like there might be a risk of interfering with the pattern recognition
that could transform into the hardware-supported method.

src/hotspot/share/utilities/moveBits.hpp line 171:

> 169: 
> 170: template <typename T, size_t N>
> 171: struct ReverseBitsImpl final : public ReverseBitsFallbackImpl<T> {};

This compiler is clang-based, so may have the built-ins.

test/hotspot/gtest/utilities/test_moveBits.cpp line 81:

> 79: 
> 80: int32_t code_quality_reverse_bytes_32(int32_t x) {
> 81:   return byteswap(x);

This belongs in test_byteswap now.

test/hotspot/gtest/utilities/test_moveBits.cpp line 89:

> 87: 
> 88: int64_t code_quality_reverse_bytes_64(int64_t x) {
> 89:   return byteswap(x);

This belongs in test_byteswap now.

-------------

PR: https://git.openjdk.org/jdk/pull/12114

More information about the hotspot-jfr-dev mailing list