RFR: 8322770: Implement C2 VectorizedHashCode on AArch64 [v2]

[Andrew Haley]

On Wed, 21 Aug 2024 14:57:24 GMT, Andrew Haley <aph at openjdk.org> wrote:

>> src/hotspot/share/utilities/intpow.hpp line 58:
>> 
>>> 56: struct intpow<T, v, 1, no_overflow> {
>>> 57:   static const T value = v;
>>> 58: };
>> 
>>> There's no need for any of this metaprogramming. A constexpr function would be better.
>> 
>> The main advantage of this implementation over one using a `constexpr` function is the ability to verify input parameter values and detect overflows at compile time using `static_assert`. This feature helps prevent user errors that could otherwise be difficult to debug. I chose this approach for its functional benefits over a `constexpr` function. If you have any suggestions on how to implement this functionality in a `constexpr` function, they would be highly welcome, as I might be missing something here. 😃
>> 
>> Below is a similar `constexpr` implementation. **Please don't accept it through the UI**. I kept a `typedef` template parameter to ensure the return value has the desired width, if needed, without applying a mask at the call site.
>> 
>> If you'd still prefer a `constexpr` function, please let me know if you have any comments on the implementation below (I'll rename it, remove `no_overflow` parameter and the comments).
>> 
>> Suggestion:
>> 
>> #include <limits>
>> 
>> template <typename T>
>> static constexpr T intpow_c(T v, unsigned p, bool no_overflow = false) {
>>   // Can't be used in constexpr function
>>   // static_assert(v || p, "0^0 is not defined");
>> 
>>   if (p == 0) {
>>     return 1;
>>   }
>> 
>>   T a = intpow_c(v, p / 2, no_overflow);
>>   T b = (p % 2) ? v : 1;
>> 
>>   // Can't be used in constexpr function
>>   // static_assert(!no_overflow || a <= std::numeric_limits<T>::max() / a, "Integer overflow");
>>   // static_assert(!no_overflow || a * a <= std::numeric_limits<T>::max() / b, "Integer overflow");
>> 
>>   return a * a * b;
>> }
>
> You're assuming a requirement that isn't really there. HotSpot calculates a great many things at startup time, and given that the arguments to intpow here are constants, there's  no possibility of any surprises at runtime. Just assert what needs to be asserted, and please keep utility functions like this as simple as possible, leaving template metaprogramming for places that need it.

So yes, this looks OK, but of course the "Can't use" comments are unnecessary if you just use` assert`s.

P.S. If T is unsigned, there is no overflow. The desired result is mod 2**n, by the definition of the unsigned integer types. 

P.P.S 0**0 is 1, for all purposes for which integer pow() is going to be used in HotSpot: this is combinatorics rather than calculus.  It's also what `Math.pow()` does, I think so we're good.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18487#discussion_r1725284490