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

[Justin King]

On Wed, 15 Feb 2023 10:47:21 GMT, Kim Barrett <kbarrett at openjdk.org> wrote:

>> 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>
>
> 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.

Done. Think I got them all.

> 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.

Yeah, they are required here based on the existing code.

> 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.

Done.

> 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.

Habit, you are correct nothing from there is used.

> 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.

Fair. Fixed.

> 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.

It was above, I moved it to be located correctly now.

> 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.

Yeah, paranoia on my part. If the STL is doing something incorrect, we have worse problems.

> 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>.

Done.

> 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.

Done.

> 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.

Fair. In other projects which expose C++ APIs to consume it's sometimes worth it to prevent people from doing odd things, so marking as final is another barrier. Here we do not have that problem.

> 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.

Hm. I believe we can. Originally I was worried about implicit integer promotion, for example if some compiler decided that uint32_t was unsigned int, but unsigned long was also the same size. It would promote unsigned long to unsigned long long causing issues. But we shouldn't have that here with the current approach.

> 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`?

Done.

> 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?

Hmm. We might be able to, let's see.

> 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`?

Done.

> 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`.

Done.

> 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.

STATIC_ASSERT is static_assert, but sets the second argument to the stringification of the first. static_assert only accepts single arguments in, I think, C++17. But since its removed here anyway, its no longer needed.

> 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.

Done.

> 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.

Out of curiosity, why `reverse_bits.hpp` instead of `reverseBits.hpp`? I have seen a few files in `utilities/` use the former, but the rest of Hotspot seems to always use the later? Is it because they contain a single function?

I went with `reverse_bits.hpp` for now as suggested for consistency with the other bit fiddling headers in `utilities/`.

> 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.

Yeah, I merge XLC with the GCC block in both. Let's see.

> 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.

Done.

> 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.

Done.

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

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