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

[Kim Barrett]

On Tue, 23 Aug 2022 17:58:39 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

Changes requested by kbarrett (Reviewer).

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

> 29: #include "utilities/globalDefinitions.hpp"
> 30: 
> 31: inline uint64_t bit_reverse(uint64_t x) {

>From the name I expect this function to reverse the bits in x, but it only reverses the bits in the individual bytes.  Please give it a better name, like `reverse_bits_in_bytes`.

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

> 29: #include "utilities/globalDefinitions.hpp"
> 30: 
> 31: inline uint64_t bit_reverse(uint64_t x) {

Throughout, do we really want to always force extension to 64bit types on a 32bit platform?

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

> 29: #include "utilities/globalDefinitions.hpp"
> 30: 
> 31: inline uint64_t bit_reverse(uint64_t x) {

It's probably worth having comments referring to the corresponding Hacker's Delight sections for both of these functions.

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

> 30: 
> 31: inline uint64_t bit_reverse(uint64_t x) {
> 32:   x = (x & 0x5555555555555555L) << 1 | (x & 0xAAAAAAAAAAAAAAAAL) >> 1;

Throughout, the `L` suffixes on the integer literals are unnecessary and potentially confusing.  (Confusing because `L` doesn't mean `long`, which would actually be wrong on Windows.)  We have `CONST64` and `UCONST64` macros in globalDefinitions.hpp that we generally use for 64bit integer literals.

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

> 36: }
> 37: 
> 38: inline uint64_t byte_reverse(uint64_t x, uint8_t bw) {

It's unclear whether this function is intended to be a public API provided by this file, or an implementation detail of reverse_bits.  It seems like it could be the former, in which case it should be documented as such, and probably reordered with bit_reverse (which likely *is* intended to just be an implementation detail for reverse_bits), and renamed to reverse_bytes for consistency.

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

> 39:   switch(bw) {
> 40:     case 64:
> 41:       x = (x & 0x00000000FFFFFFFFL) << 32 | (x & 0xFFFFFFFF00000000L) >> 32;

Here's a case where the `L` suffix doesn't denote a 64 bit literal on Windows (because it is LLP64), though it doesn't matter because of the immediate implicit conversion with the type of `x`.

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

> 45:       x = (x & 0x00FF00FF00FF00FFL) << 8 | (x & 0xFF00FF00FF00FF00L) >> 8;
> 46:     default:
> 47:       break;

Rather than assuming the only uncovered case is 8, I think better would be to have an explicit case for 8 and a default ShouldNotReachHere case.

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

> 50: }
> 51: 
> 52: template <typename T, uint8_t S> struct ReverseBitsImpl {};

There's no benefit to using uint8_t for the size.  The type of the result of `sizeof` is `size_t` - just use that.

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?

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

> 65: template <typename T> struct ReverseBitsImpl<T, 2> {
> 66:   static T doit(T v) {
> 67:     uint64_t r = bit_reverse((uint64_t)v);

Throughout, implicit conversion of integral types will deal with the argument to `bit_reverse`, so there's no need to cast the argument being passed to it.

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

> 112: // Performs bit reversal of a multi-byte type, we implement and support
> 113: // variants for 8, 16, 32 and 64 bit integral types.
> 114: template <typename T> inline T reverse_bits(T v) {

Restrict to integral types, using `ENABLE_IF(std::is_integral<T>::value)`.

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

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