RFR: 8291649: multiple tests failing with -Xcomp after JDK-8290034 [v2]

[Kim Barrett]

On Fri, 26 Aug 2022 01:06:25 GMT, John R Rose <jrose at openjdk.org> wrote:

>> src/hotspot/share/utilities/moveBits.hpp line 31:
>> 
>>> 29: #include "utilities/globalDefinitions.hpp"
>>> 30: 
>>> 31: inline uint32_t reverse_bits_in_bytes_int(uint32_t x) {
>> 
>> Instead of two functions with different names (suffixed `_int` and `_long`), have one function name that has two overloads disambiguated by the size of the argument, e.g.
>> 
>> 
>> template<typename T, ENABLE_IF(sizeof(T) <= 4)>
>> constexpr T reverse_bits_in_bytes(T x) { ... }
>> 
>> template<typename T, ENABLE_IF(sizeof(T) == 8)>
>> constexpr T reverse_bits_in_bytes(T x) { ... }
>> 
>> 
>> This allows `reverse_bits` to look something like
>> 
>> 
>> template<typename T, ENABLE_IF(std::is_integral<T>::value)>
>> constexpr T reverse_bits(T v) {
>>   return reverse_bytes(reverse_bits_in_bytes(v));
>> }
>
> Yes, that could work.
> 
> This also works, and feels appropriately short + sweet to me:
> 
> https://github.com/rose00/jdk/pull/new/moveBits
> 
> (I also added a 90+% level gtest.)
> 
> (Sorry; amended with `constexpr` and force-pushed…)

I took John's code and made various (I think) improvements:
https://github.com/openjdk/jdk/compare/master...kimbarrett:openjdk-jdk:reverse-bits?expand=1

>> src/hotspot/share/utilities/moveBits.hpp line 63:
>> 
>>> 61: /*****************************************************************************
>>> 62:  * GCC and compatible (including Clang)
>>> 63:  *****************************************************************************/
>> 
>> Is it really worth the extra code to allow the use of the gcc builtins?
>
> Kim, is there a way to do this in less code?
> 
> Seems to me that something like this could work:
> 
> 
> template<typename T, size_t S>
> T reverse_bytes(T x) {
>   if (size == 1)  return x;
>   T alt8bs = (T)((uint64_t)-1 / 0xFFFF * 0xFF);  // 0x00FF…00FF
>   x = ((x & alt8bs) << 8) | (((uint64_t)x >> 8 & alt8bs);
>   if (size == 2)  return x;
>   T alt16bs = (T)((uint64_t)-1 / 0xFFFFFFFFll * 0xFFFF);  // 0x0000FFFF…0000FFFF
>   …
> }
> 
> 
> That is, one function bodies handles cases for 1/2/4/8 byte values.
> 
> Could it be just a function template, with an explicit instantiation (overriding the general template) where it matches an intrinsic?

Answering my own question, no, it's not worth the extra code.  gcc (and likely other compilers) are quite good at recognizing byte swapping code and transforming it into the appropriate platform-specific code like x86 bswap.

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

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