RFR: 8343685: C2 SuperWord: refactor VPointer with MemPointer [v4]

[Vladimir Kozlov]

On Tue, 14 Jan 2025 06:43:26 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> src/hotspot/share/opto/mempointer.hpp line 677:
>> 
>>> 675:     assert(pos == summands.length(), "copied all summands");
>>> 676: 
>>> 677:     assert(1 <= _size && _size <= 2048 && is_power_of_2(_size), "valid size");
>> 
>> Where 2048 comes from? Do you have a runtime check somewhere too?
>
> It is just a sanity check: currently no platform has vectors larger than 2048. Replaced the assert with:
> `assert(1 <= _size && _size <= 2048 && is_power_of_2(_size), "sanity: no vector is expected to be larger");`

Okay.

>> src/hotspot/share/opto/noOverflowInt.hpp line 109:
>> 
>>> 107:     } else if (b.is_NaN()) {
>>> 108:       return -1;
>>> 109:     }
>> 
>> This is strange NaN compare results. May be add comment explaining that it is not really float arithmetic "NaN".
>
> @vnkozlov At the top of the file I explain the meaning of `NaN`:
> 
> // Wrapper around jint, which detects overflow.
> // If any operation overflows, then it returns a NaN.
> class NoOverflowInt {
> private:
>   bool _is_NaN; // overflow, uninitialized, etc.
>   jint _value;
> 
> 
> Is that sufficient? Or would you prefer me to rename the int `NaN` to something else?
> 
> I added a comment line now, I hope that helps locally:
> 
>   static int cmp(const NoOverflowInt& a, const NoOverflowInt& b) {
>     // Order NaN (overflow, uninitialized, etc) after non-NaN.
>     if (a.is_NaN()) {
>       return b.is_NaN() ? 0 : 1;
>     } else if (b.is_NaN()) {
>       return -1;
>     }
>     if (a.value() < b.value()) { return -1; }
>     if (a.value() > b.value()) { return  1; }
>     return 0;
>   }

Naming aside (I can accept it) it is weird that `NaN` have ordering with normal `values` (based on `cmp()` code): it looks like any `NaN is bigger any `value` and any `NaN` is equal to any `NaN`.  It is may be fine if it is used only for sorting to move NaNs to the end of a list. My concern is it may introduce issue if it is used without care.

I asked about comment which would explain your NaNs in this matter because float arithmetic NaNs compare rules are different.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1916004310
PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1916003500