8347645: C2: XOR bounded value handling blocks constant folding #23089

j3graham · 2025-01-13T22:16:20Z

An interaction between xor bounds optimization and constant folding resulted in xor over constants not being optimized. This has a noticeable effect on Long.expand with a constant mask, on architectures that don't have instructions equivalent to PDEP to be used in an intrinsic.

This change moves logic from the Xor(L|I)Node::Value methods into the add_ring methods, and gives priority to constant-folding. A static method was separated out to facilitate direct unit-testing. It also (subjectively) simplified the calculation of the upper bound and added an explanation of the reasoning behind it.

In addition to testing for constant folding over xor, IR tests were added to XorINodeIdealizationTests and XorLNodeIdealizationTests to cover these related items:

Bounds optimization of xor
A check for x ^ x = 0
Explicit testing of xor over booleans.

Also test_xor_node.cpp was added to more extensively test the correctness of the bounds optimization. It exhaustively tests ranges of 4-bit numbers as well as at the high and low end of the affected types.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8347645: C2: XOR bounded value handling blocks constant folding (Bug - P4)

Reviewers

Quan Anh Mai (@merykitty - Committer) 🔄 Re-review required (review applies to cf779497)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23089

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8347645: C2: XOR bounded value handling blocks constant folding (Bug - P4)

Reviewers

Quan Anh Mai (@merykitty - Committer) 🔄 Re-review required (review applies to cf779497)
Jasmine Karthikeyan (@jaskarth - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23089

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

Using diff file

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

Using Webrev

Link to Webrev Comment

j3graham · 2025-01-13T22:16:38Z

Sample program:

public class XorTest {
    public static void main(String[] args) {
        long t=0;
        for (int i = 0; i < 10_000; i++) {
            t+=doXor();
        }
        System.out.println("t = " + t);
    }

    static long doXor(){
        long c=42;
        return c ^ 2025L;
    }
}

IGV before patch (constant nodes are red)

IGV after patch

bridgekeeper · 2025-01-13T22:17:05Z

👋 Welcome back j3graham! 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 · 2025-01-13T22:17:15Z

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

openjdk · 2025-01-13T22:17:49Z

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

SirYwell · 2025-01-14T07:27:39Z

The code for this optimization seems to be present already in the add_ring methods, and from my understanding those should be called by AddNode::Value. Does that mean XorINode::Value and XorLNode::Value never call that method in your tests, and instead always end up in the if directly below your changes? Maybe that if should rather check to ignore constant values? Alternatively, that if could be moved into the add_ring methods too maybe?

liach · 2025-01-14T08:24:50Z

Created https://bugs.openjdk.org/browse/JDK-8347645 for you. The causes are #2776 and #4136.

I think the right way forward is to remove XorI/LNode::Value and move the code to their add_ring, which has lower priority than constant folding, instead. However I am not a hotspot/JIT engineer, so don't take my words for granted.

j3graham · 2025-01-14T18:12:27Z

Thanks for creating the bug.

I have left the x ^x =0 check in Value because it was operating on the Node rather than the Type. I moved the rest into add_ring.

Done for Int, Long to follow.

mlbridge · 2025-01-15T21:38:50Z

Webrevs

eme64 · 2025-01-22T07:44:15Z

@j3graham Since this is a performance improvement: do you have any benchmark that shows a speedup?

eme64

Thanks for working on this!

I have not yet looked at the VM cpp changes yet.

Some comments about the test:
Please move it to:
test/hotspot/jtreg/compiler/c2/gvn/TestXor.java

The comments sometimes mention c3 etc, but there may only be a c or x. Please fix them ;)

The tests should also do result verification. Currently you only check that we have the expected nodes, but constant folding could have bugs we would not catch this way. What I usually do:
Compute some GOLDEN, which should be computed in interpreter, and then with a @Check method you can compare the result to that GOLDEN value.

Plus: it would be nice if the constants could be picked at random. You can do that with a public static final int CON = random_value.

Best would be if you could use the new Generators, see
./test/hotspot/jtreg/compiler/lib/generators/Generators.java

Let me know if you need any more help with that.

openjdk · 2025-01-22T17:09:04Z

⚠️ @j3graham This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

j3graham · 2025-01-22T17:11:06Z

Thanks for the feedback. I've updated the tests as suggested.

src/hotspot/share/opto/addnode.hpp

test/hotspot/gtest/opto/test_xor_node.cpp

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java

reorg gtest

merykitty · 2025-02-01T09:17:38Z

Very nice, I think the patch looks good, please do another round of style refinement. In particular, make sure that there is no white space after ( or before ), and after if or for we prefer having a whitespace before the (.

j3graham · 2025-02-01T19:31:00Z

Thanks. I've done another round of format fixing. I've also simplified the IR tests so they don't try to cover as much as gtest does, and added equivalent tests for long.

I have temporarily left the more elaborate tests commented out in XorINodeIdealizationTests. I will remove them if nobody thinks they are worth keeping.

merykitty

Otherwise LGTM, very nice tests, thanks very much!

src/hotspot/share/opto/addnode.cpp

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java

eme64 · 2025-02-13T08:37:49Z

@j3graham Can you please update the PR description at the top? The current version does not reflect the most up-to-date explanations, right?

I would like to see a nice summary, what cases were covered before and what cases you are now covering additionally.
Give a quick explanation how you changed the code.

I'll increase the number of reviewers as this looks like a substantial change.

/reviewers 2 reviewer

openjdk · 2025-02-13T08:38:37Z

@eme64
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

eme64 · 2025-02-13T08:43:39Z

I also see that #2776 and #4136 were mentioned here. Both of those are related an have no IR tests of their own, yikes! We have to ensure that we cover those old cases, and then new ones here, so that we do not get any accidental regressions.

Maybe that's all already covered in other existing tests or the tests you added. Can you please provide a summary of all tests and what cases they cover in the PR description? It would help a lot for reviewing.

eme64

@j3graham Thanks for taking this on! It's great to see someone clean this up and make sure all the cases optimize as expected!

src/hotspot/share/opto/addnode.cpp

j3graham · 2025-02-13T21:04:46Z

I have updated the summary to be more informative. I believe the related optimizations are now covered in tests.

jaskarth

This looks really nice! I just have 2 small comments here.

src/hotspot/share/opto/addnode.cpp

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java

Co-authored-by: Jasmine Karthikeyan <[email protected]>

jaskarth

Thanks for the update! It looks good to me.

j3graham · 2025-02-24T18:20:24Z

Hi, @eme64, do you have any additional comments on this PR?

j3graham added 2 commits January 13, 2025 16:50

test for xor const folding

621f0d8

xor const folding

6b4e80d

openjdk bot added the hotspot-compiler [email protected] label Jan 13, 2025

j3graham mentioned this pull request Jan 13, 2025

Speed up UUID::toString #22928

Open

3 tasks

format, copyright date

21979d9

move logic from Value to add_node XorI

7cd18f8

j3graham changed the title ~~C2 does not do constant-folding of xor~~ 8347645: C2: XOR bounded value handling blocks constant folding Jan 14, 2025

move logic from Value to add_node XorL

8ec4313

j3graham marked this pull request as ready for review January 15, 2025 21:34

openjdk bot added the rfr Pull request is ready for review label Jan 15, 2025

eme64 reviewed Jan 22, 2025

View reviewed changes

j3graham added 7 commits January 22, 2025 10:36

test for xor const folding

bbdad2c

xor const folding

17dcb39

format, copyright date

3f5f5aa

move logic from Value to add_node XorI

862db73

move logic from Value to add_node XorL

55f4b26

Merge remote-tracking branch 'origin/xor_const' into xor_const

1e877ae

move tests, add correctness checks

fe3b428

Merge branch 'openjdk:master' into xor_const

fdede51

j3graham added 3 commits January 31, 2025 00:40

fix int size

d445cf4

retry templated tests

bf2fcfd

move template def to header

3e460f9

merykitty reviewed Jan 31, 2025

View reviewed changes

j3graham added 5 commits January 31, 2025 11:44

Merge branch 'openjdk:master' into xor_const

5ee3a0c

move template def to cpp

d7bca8a

reorg gtest

try fewer tests

112bcca

re-add tests

2c28f3e

add sanity asserts to tests

9effc3d

j3graham added 2 commits February 1, 2025 12:05

formatting

ab804b8

add IR tests for long, simplify tests for int

cf77949

merykitty approved these changes Feb 12, 2025

View reviewed changes

src/hotspot/share/opto/addnode.cpp Outdated Show resolved Hide resolved

src/hotspot/share/opto/addnode.cpp Outdated Show resolved Hide resolved

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java Outdated Show resolved Hide resolved

formatting, remove commented tests

4a29120

eme64 suggested changes Feb 13, 2025

View reviewed changes

j3graham added 2 commits February 13, 2025 14:58

address review comments

0e9b7cb

update test

a522898

j3graham added 2 commits February 13, 2025 16:13

fix variable names in comments

42b20b5

Merge branch 'openjdk:master' into xor_const

16049cd

jaskarth reviewed Feb 18, 2025

View reviewed changes

src/hotspot/share/opto/addnode.cpp Outdated Show resolved Hide resolved

test/hotspot/jtreg/compiler/c2/irTests/XorINodeIdealizationTests.java Outdated Show resolved Hide resolved

j3graham and others added 2 commits February 17, 2025 21:37

Fix formatting

e8fc6da

Co-authored-by: Jasmine Karthikeyan <[email protected]>

update tests

40b1f9c

jaskarth approved these changes Feb 21, 2025

View reviewed changes

j3graham requested a review from merykitty February 21, 2025 03:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8347645: C2: XOR bounded value handling blocks constant folding #23089

8347645: C2: XOR bounded value handling blocks constant folding #23089

j3graham commented Jan 13, 2025 •

edited by openjdk bot

Loading

j3graham commented Jan 13, 2025

bridgekeeper bot commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

SirYwell commented Jan 14, 2025 •

edited

Loading

liach commented Jan 14, 2025

j3graham commented Jan 14, 2025

mlbridge bot commented Jan 15, 2025 •

edited

Loading

eme64 commented Jan 22, 2025

eme64 left a comment

openjdk bot commented Jan 22, 2025

j3graham commented Jan 22, 2025

merykitty commented Feb 1, 2025

j3graham commented Feb 1, 2025

merykitty left a comment

eme64 commented Feb 13, 2025

openjdk bot commented Feb 13, 2025

eme64 commented Feb 13, 2025

eme64 left a comment

j3graham commented Feb 13, 2025

jaskarth left a comment

jaskarth left a comment

j3graham commented Feb 24, 2025 •

edited

Loading

8347645: C2: XOR bounded value handling blocks constant folding #23089

Are you sure you want to change the base?

8347645: C2: XOR bounded value handling blocks constant folding #23089

Conversation

j3graham commented Jan 13, 2025 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Progress

Issue

Reviewers

Reviewing

j3graham commented Jan 13, 2025

bridgekeeper bot commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

openjdk bot commented Jan 13, 2025

SirYwell commented Jan 14, 2025 • edited Loading

liach commented Jan 14, 2025

j3graham commented Jan 14, 2025

mlbridge bot commented Jan 15, 2025 • edited Loading

Webrevs

eme64 commented Jan 22, 2025

eme64 left a comment

Choose a reason for hiding this comment

openjdk bot commented Jan 22, 2025

j3graham commented Jan 22, 2025

merykitty commented Feb 1, 2025

j3graham commented Feb 1, 2025

merykitty left a comment

Choose a reason for hiding this comment

eme64 commented Feb 13, 2025

openjdk bot commented Feb 13, 2025

eme64 commented Feb 13, 2025

eme64 left a comment

Choose a reason for hiding this comment

j3graham commented Feb 13, 2025

jaskarth left a comment

Choose a reason for hiding this comment

jaskarth left a comment

Choose a reason for hiding this comment

j3graham commented Feb 24, 2025 • edited Loading

j3graham commented Jan 13, 2025 •

edited by openjdk bot

Loading

SirYwell commented Jan 14, 2025 •

edited

Loading

mlbridge bot commented Jan 15, 2025 •

edited

Loading

j3graham commented Feb 24, 2025 •

edited

Loading