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

[John Rose]

Thanks; right.  I like your improvements, except the metaprogramming.

Here’s what I got:

https://github.com/openjdk/jdk/compare/master...rose00:jdk:moveBits

I’m not against metaprogramming when we need it, but there are 
alternatives here to deriving the extra types; the tricks are explained 
in the comments.  I suppose it’s a matter of taste whether the 
metaprogramming is more or less obscure than the bit-twiddling tricks, 
but I will observe that bit-twiddling tricks are the focus of this file.

I verified by hand that you get good code in all cases, even without 
explicit mentions of the intrinsics.  Here’s an easy way to start that 
process:

```
$ make hotspot exploded-test TEST=gtest:moveBits
$ objdump -D $(find build -name test_moveBits.o) > /tmp/foo
$ sed < /tmp/foo -n /quality/,/stream/p | grep -B20 bswap

```


On 25 Aug 2022, at 23:39, Kim Barrett wrote:

> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-dev/attachments/20220826/b9da1295/attachment-0001.htm>