RFR: 8370794: C2 SuperWord: Long/Integer.compareUnsigned return wrong value for EQ/NE in SLP

[Hamlin Li]

On Wed, 29 Oct 2025 16:52:10 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you help to review this patch?
>> 
>> [JDK-8370481](https://bugs.openjdk.org/browse/JDK-8370481) introduces this regression for unsigned I/L EQ/NE in SLP.
>> 
>> ====================
>> 
>> In [JDK-8370481](https://bugs.openjdk.org/browse/JDK-8370481), we fixed an issue related to transformation from (Bool + CmpU + CMove) to (VectorMaskCmp + VectorBlend), and added tests for unsigned ones.
>> As discussion in [1], we should also add more tests for transformation from (Bool + Cmp + CMove) to (VectorMaskCmp + VectorBlend) for the signed ones.
>> 
>> [1] https://github.com/openjdk/jdk/pull/27942#discussion_r2468750039
>> 
>> Thanks!
>
> @eme64 When I develop this test pr, found out that, after https://github.com/openjdk/jdk/pull/27942 there could be some failing scenarios for unsigned EQ/NE (found by the newly developed unsigned EQ/NE test). We could fix it with the following patch, I can put the fix patch and related EQ/NE signed/unsigned tests in this pr, but seems it's better to put them in another separate pr? Please let me know what do you think. Thanks
> 
> 
> diff --git a/src/hotspot/share/opto/subnode.cpp b/src/hotspot/share/opto/subnode.cpp
> index 9c6c7498dd0..a10cd2a8d5b 100644
> --- a/src/hotspot/share/opto/subnode.cpp
> +++ b/src/hotspot/share/opto/subnode.cpp
> @@ -1398,6 +1398,21 @@ const Type *BoolTest::cc2logical( const Type *CC ) const {
>    return TypeInt::BOOL;
>  }
>  
> +BoolTest::mask BoolTest::unsigned_mask(BoolTest::mask btm) {
> +  switch(btm) {
> +    case eq:
> +    case ne:
> +      return btm;
> +    case lt:
> +    case le:
> +    case gt:
> +    case ge:
> +      return mask(btm | unsigned_compare);
> +    default:
> +      ShouldNotReachHere();
> +  }
> +}
> +
>  //------------------------------dump_spec-------------------------------------
>  // Print special per-node info
>  void BoolTest::dump_on(outputStream *st) const {
> diff --git a/src/hotspot/share/opto/subnode.hpp b/src/hotspot/share/opto/subnode.hpp
> index 2c3d9cfd35e..463d9e020cb 100644
> --- a/src/hotspot/share/opto/subnode.hpp
> +++ b/src/hotspot/share/opto/subnode.hpp
> @@ -331,7 +331,7 @@ struct BoolTest {
>    mask negate( ) const { return negate_mask(_test); }
>    // Return the negative mask for the given mask, for both signed and unsigned comparison.
>    static mask negate_mask(mask btm) { return mask(btm ^ 4); }
> -  static mask unsigned_mask(mask btm) { return mask(btm | unsigned_compare); }
> +  static mask unsigned_mask(mask btm);
>    bool is_canonical( ) const { return (_test == BoolTest::ne || _test == BoolTest::lt || _test == BoolTest::le || _test == BoolTest::overflow); }
>    bool is_less( )  const { return _test == BoolTest::lt || _test == BoolTest::le; }
>    bool is_greater( ) const { return _test == BoolTest::gt || _test == BoolTest::ge; }

> @Hamlin-Li By failing you mean they now produce a wrong result? If yes, maybe we can convert this issue here to a bugfix? And just add all the additional tests to ensure we don't have any other bugs.
> 
> Is it just a regression from the last patch, or an older issue? Probably just a regression from #27942, because before we just did nothing with the mask, and that is what you want to go back to, right?

Yes, it's a regression from #27942.
I'll update the bug and pr, also add fix and tests for unsigned EQ/NE.

> BTW: thanks for writing all the tests, it seems to be the only way to ensure we get it all right :)

Thanks for the quick response! :)

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

PR Comment: https://git.openjdk.org/jdk/pull/28047#issuecomment-3462781362