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

[Emanuel Peter]

On Thu, 23 Oct 2025 10:16:39 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you help to review the patch? @eme64 
>> 
>> Currently, in SLP if we support transformation from (Bool + CmpU + CMove) to (VectorMaskCmp + VectorBlend), the unsigned comparison information is lost, it's in CmpU, but current code only check Bool for the information. For details please check code at `SuperWordVTransformBuilder::make_vector_vtnode_for_pack` and `PackSet::get_bool_test`.
>> 
>> This loss of unsigned comparison information blocks the optimization proposed in https://github.com/openjdk/jdk/pull/25336 and https://github.com/openjdk/jdk/pull/25341.
>> 
>> Currently, `BoolTest` does not support an unsigned construction (`BoolTest( mask btm ) : _test(btm) { assert((btm & unsigned_compare) == 0, "unsupported");}`), seems to me a feasible solution would be get the unsigned information from CmpU (which could be an input of Bool) and pass it to VectorMaskCmp.
>> 
>> Thanks
>
> Hamlin Li has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - tests
>  - switch

You should also change the PR description, especially you should describe what went wrong at what point.


Well, you mostly already explain. I think the issue is that we don't really carry the "unsigned-ness" of the comparison, and then end up doing signed instead of unsigned comparison...

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.

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?

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

PR Comment: https://git.openjdk.org/jdk/pull/27942#issuecomment-3436400459
PR Review Comment: https://git.openjdk.org/jdk/pull/27942#discussion_r2454782958
PR Review Comment: https://git.openjdk.org/jdk/pull/27942#discussion_r2454786435