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

[Emanuel Peter]

On Wed, 15 Jan 2025 06:26:10 GMT, Vladimir Kozlov <kvn at openjdk.org> wrote:

>> @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.

I now put this comment:

+  // This "cmp" is used for sort only.
+  // Note: the NaN semantics are different from floating arithmetic NaNs!
+  // - Smaller non-NaN are before larger non-NaN.
+  // - Any non-NaN are before NaN.
+  // - NaN is equal to NaN.
+  // Note: NaN indicate overflow, uninitialized, etc.

>> I cannot make good conditions currently, sadly.
>> IR `applyIf` can rely on VM flags, CPU features etc.
>> 
>> But in my case, it would pass for `native` memory, but not for `array` cases. And that is decided by the test command line arguments of the runs. It gets passed in like `-DmemorySegmentProviderNameForTestVM=Native` or `-DmemorySegmentProviderNameForTestVM=ByteArray` etc.
>> 
>> This one is array, and for some reason does not currently parse pointers sufficiently well to vectoirze:
>> ` * @run driver compiler.loopopts.superword.TestMemorySegment ByteArray`
>> But this is native, and vectorizes:
>> ` * @run driver compiler.loopopts.superword.TestMemorySegment Native`
>> 
>> @vnkozlov @chhagedorn is there any way I can currently do an `applyIf` for that?
>> 
>> I could remove the IR rule rather than comment if, if that is better for you.
>
> In case you want to fix test method later or modify it (have separate tests for native and java arrays) I would prefer to have IR testing rule which can say to "skip" (not run) a test without need to comment out it and its rules.
> May be with text explaining why test skipped.
> You can delete it if you don't want to do that.

Ok, I will remove the commented IR rule for now. The follow-up bug is already filed: JDK-8330274

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

PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1916012947
PR Review Comment: https://git.openjdk.org/jdk/pull/21926#discussion_r1916012525