8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP

[Hamlin-Li]

Hi,
Can you help to review the patch? @eme64

Issue

Currently, in SLP when transform from (Bool + CmpU + CMove) to (VectorMaskCmp + VectorBlend), the unsigned-ness in CmpU is lost, then end up doing a signed instead of unsigned comparison in VectorMaskCmp.
For details please check code at SuperWordVTransformBuilder::make_vector_vtnode_for_pack and PackSet::get_bool_test.

 Fix

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

This pr could also lead to more optimizations, like: #25336 and #25341.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP (Bug - P3)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/27942/head:pull/27942
$ git checkout pull/27942

Update a local copy of the PR:
$ git checkout pull/27942
$ git pull https://git.openjdk.org/jdk.git pull/27942/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 27942

View PR using the GUI difftool:
$ git pr show -t 27942

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/27942.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@Hamlin-Li The following label will be automatically applied to this pull request:

• hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 05: Full - Incremental (309880d8)
• 04: Full - Incremental (650f9200)
• 03: Full - Incremental (34fafd2c)
• 02: Full - Incremental (2bad7aea)
• 01: Full - Incremental (eee8acd8)
• 00: Full (6271a8e7)

[eme64]

@Hamlin-Li That looks like a great improvement :)

All I am missing are some test cases. And if possible: IR rules 😉

[Hamlin-Li]

@Hamlin-Li That looks like a great improvement :)

Thank you for having a look! :)

All I am missing are some test cases. And if possible: IR rules 😉

Yes, I also added some tests and IR rules of course. :)

[Hamlin-Li]

/solves JDK-8370481

[openjdk]

@Hamlin-Li
Adding additional issue to solves list: 8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP.

[eme64]

@Hamlin-Li
I just looked at your Bug report JDK-8370481.
Can you add a reproducer, and on what platforms you have experienced the failure?

If you are fixing a bug here, we should focus on the bug only. After all, we may want to backport the bugfix.

[Hamlin-Li]

@Hamlin-Li I just looked at your Bug report JDK-8370481. Can you add a reproducer, and on what platforms you have experienced the failure?

Please check the added test cases, if it's run with JDK master, the test cases will fail on x86.

If you are fixing a bug here, we should focus on the bug only. After all, we may want to backport the bugfix.

Yes. In fact at first I did not realise the existence of the bug. After read your previous question, I did more investigation and found out that I'm working a bug fix at the same time. I can change the title of this pr to "8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP" if you wish.
Maybe I should close JDK-8370454 at the same time?
Please let me know.

[eme64]

@Hamlin-Li Nice, thanks for filing JDK-8370481. I now extracted a reproducer from your PR here, and attached it to the JIRA issue.

Yes, I think it would be better if you changed the title to:
8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP

I think you could already close JDK-8370454 as a duplicate of the bug report :)

Thanks very much for finding this, and even more for fixing it 😊

[eme64]

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

[Hamlin-Li]

@Hamlin-Li Nice, thanks for filing JDK-8370481. I now extracted a reproducer from your PR here, and attached it to the JIRA issue.

Thank you for doing it!

Yes, I think it would be better if you changed the title to: 8370481: C2 SuperWord: Long/Integer.compareUnsigned return wrong value in SLP

I think you could already close JDK-8370454 as a duplicate of the bug report :)

It's done!

Thanks very much for finding this, and even more for fixing it 😊

Thank you! :)

[Hamlin-Li]

@eme64 I just keep Op_CmpI/L.
Test running, will update the result or code accordingly later.

[eme64]

I just realized that we only have GT cases. But what about GE,LE,LT? Are those covered somewhere else?

Otherwise we only test a fraction of
static mask unsigned_mask(mask btm) { return mask(btm | unsigned_compare); }

What do you think?

[Hamlin-Li]

Make sense, I'll add more tests.

[Hamlin-Li]

Added tests to cover UI{GE|LT|LE}forF and UL{GE|LT|LE}forD.

Other tests for example UI{GE|LT|LE}forD UL{GE|LT|LE}forF could be added when I work on #25336 or #25341, as currently they are not vectorized.

[Hamlin-Li]

@eme64 I just keep Op_CmpI/L. Test running, will update the result or code accordingly later.

I ran all the tests under test/hotspot/jtreg/compiler on x86, the default case (i.e. ShouldNotReachHere) is not triggerred. Plus github CI, I think we are good at these switch cases.