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

Kim Barrett kbarrett at openjdk.org
Fri Aug 26 01:04:03 UTC 2022


On Thu, 25 Aug 2022 19:06:54 GMT, Jatin Bhateja <jbhateja at openjdk.org> wrote:

>> Hi,
>> 
>> This bug fix patch fixes the incorrect computation during constant folding of Reverse[IL] IR node.
>> A new utility class has been added to optimize computation of reverse_bit operation using GCC built-ins.
>> 
>> Kindly review and share your feedback.
>> 
>> Best Regards,
>> Jatin
>
> Jatin Bhateja has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8291649: Review comments resolutions.

Changes requested by kbarrett (Reviewer).

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

> 28: #include "utilities/debug.hpp"
> 29: #include "utilities/globalDefinitions.hpp"
> 30: 

I think everything here should be constexpr.

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));
}

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

> 34:   x = (x & 0x33333333) << 2 | (x & 0xCCCCCCCC) >> 2;
> 35:   x = (x & 0x0F0F0F0F) << 4 | (x & 0xF0F0F0F0) >> 4;
> 36:   return x;

You missed the point.  Hacker's Delight (and seander) use shift-and-mask on one side of the bit-or and mask-and-shift on the other so the same constant is used on both sides.  See
https://graphics.stanford.edu/~seander/bithacks.html#ReverseParallel
Similarly in the byte swappers.

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

> 100:   static T doit(T v) {
> 101:     v = reverse_bits_in_bytes_int(v);
> 102:     return reverse_bytes_int(r, 16);

I think the use of reverse_bytes_int is wrong here.

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

> 106: template <typename T> struct ReverseBitsImpl<T, 4> {
> 107:   static T doit(T v) {
> 108:     v = reverse_bits_in_bytes(v);

I don't see this function.  Similarly in the size==8 case.

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

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


More information about the hotspot-compiler-dev mailing list