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