RFR: 8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP [v2]

[Emanuel Peter]

On Thu, 23 Oct 2025 12:38:11 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> src/hotspot/share/opto/superword.cpp line 1755:
>> 
>>> 1753:     mask = BoolTest::unsigned_mask(mask);
>>> 1754:     break;
>>> 1755:   } // switch
>> 
>> Given that we just missed some cases in the old "if": you should definitively have a `default` case, that hits an assert. That way, we don't have missing cases that silently do the wrong thing.
>
> Not sure I understand you correctly.
> Do you mean add some code like below?
> 
> @@ -1752,6 +1752,8 @@ VTransformBoolTest PackSet::get_bool_test(const Node_List* bool_pack) const {
>    case Op_CmpUL3:
>      mask = BoolTest::unsigned_mask(mask);
>      break;
> +  default:
> +    ShouldNotReachHere(); // or assert
>    } // switch
> 
> But besides of Op_CmpF/D, Op_CmpUxx, there could be other Cmp ops call into `PackSet::get_bool_test`.
> 
> Or maybe I misunderstood you?

There could be, but then we should explicitly name them, and make sure that they are all ok.
Just see what cases you need to add, until all testing passes. I hope it would only be a handfull of cmp nodes.

Doing it all explicitly means we know better that we thought about all cases.

>> test/hotspot/jtreg/compiler/c2/irTests/TestVectorConditionalMove.java line 743:
>> 
>>> 741:             r[i] = Integer.compareUnsigned(a[i], b[i]) > 0 ? cc : dd;
>>> 742:         }
>>> 743:     }
>> 
>> Why does this not vectorize, but `testCMoveUIGTforF` does? Is that not strange?
>
> These test cases are converted from signed ones above, for example `testCMoveUIGTforI` is from `testCMoveIGTforI`.
> 
> To answer your question, I think the reason is at the comment above `testCMoveIGTforI`.
> In https://github.com/openjdk/jdk/pull/25336, I'm going to enable this vectorization, but previously https://github.com/openjdk/jdk/pull/25336 was blocked by unsigned comparison issue (check the discussion at: https://github.com/openjdk/jdk/pull/25336#discussion_r2123518238).
> With this pr pushed in, I think I can restart the work of https://github.com/openjdk/jdk/pull/25336.

Hmm ok, sounds good.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/27942#discussion_r2455122473
PR Review Comment: https://git.openjdk.org/jdk/pull/27942#discussion_r2455127584