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

Emanuel Peter (@eme64 - Reviewer)
Quan Anh Mai (@merykitty - Committer) Review applies to cf779497
Jasmine Karthikeyan (@jaskarth - Committer) Review applies to 40b1f9c4
Vladimir Ivanov (@iwanowww - Reviewer)

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

@j3graham This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8347645: C2: XOR bounded value handling blocks constant folding

Reviewed-by: epeter, vlivanov, qamai, jkarthikeyan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 96 new commits pushed to the master branch:

b92a443: 8353471: ZGC: Redundant generation id in ZGeneration
cfcb330: 8353681: jpackage suppresses errors when executed with --verbose option
10d1fec: 8353679: Restructure classes in jdk.jpackage.internal package
1161b56: 8353053: (fs) Add support for UserDefinedFileAttributeView on AIX
a13e34d: 8353274: [PPC64] Bug related to -XX:+UseCompactObjectHeaders -XX:-UseSIGTRAP in JDK-8305895
41d4a0d: 8352392: AIX: implement attach API v2 and streaming output
1c2a553: 8327858: Improve spliterator and forEach for single-element immutable collections
a449aee: 8350704: Create tests to ensure the failure behavior of core reflection APIs
57df89c: 8353684: [BACKOUT] j.u.l.Handler classes create deadlock risk via synchronized publish() method
ebcb9a8: 8349206: j.u.l.Handler classes create deadlock risk via synchronized publish() method
... and 86 more: https://git.openjdk.org/jdk/compare/3d2c3cd40ebce901d09a2479c267342e04e6f79c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@eme64, @merykitty, @jaskarth, @iwanowww) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

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.

iwanowww · 2025-03-31T22:28:49Z

Thanks.

The naming of that method evolved during the course of the review of this PR. I believe the thinking was that the check was not necessarily an overall upper bound, and a simpler name would imply it was more general.

There's usually a lot of invariants a function assumes and it's simply impractical to encode everything in the name. Speaking of this particular case (calc_xor_upper_bound_of_non_neg):

calc_ is redundant and IMO only adds noise;
_non_neg part is confusing; I'd stress instead that it works on ranges.

So, xor_upper_bound_for_ranges then? (And, please, explain in the comment what's the correspondense between S and U template type parameters.)

addnodeXorUtil.hpp

I'm fine with placing it under opto. Please, rename the file into src/hotspot/share/opto/utilities/xor.hpp.

merykitty · 2025-04-01T02:14:35Z

@iwanowww

_non_neg part is confusing; I'd stress instead that it works on ranges.

I find it easier to think of it as calculating the upperbound of the xor of 2 non-negative integers whose upperbounds are given in the parameters.

j3graham · 2025-04-01T02:49:45Z

Renamed to xor_upper_bound_for_ranges before I saw your comment, @merykitty. I'd be ok with another name though. With the last changes, the method is no longer a member of the class, so it's no longer going to get as many eyes on it without context, so maybe it matters less now.

iwanowww

Overall, looks good.

Some minor comments follow.

src/hotspot/share/opto/addnode.cpp

iwanowww

Looks good!

eme64 · 2025-04-02T06:16:26Z

@j3graham I gave it a quick look, and it looks even better now. Let me run testing again before you integrate!

Please ping me in 24h for the results!

j3graham · 2025-04-03T17:01:05Z

Hi @eme64, any news on test results?

eme64

Testing looks good :)

wenshao · 2025-04-04T08:27:11Z

/sponsor

openjdk · 2025-04-04T08:27:47Z

@wenshao The change author (@j3graham) must issue an integrate command before the integration can be sponsored.

j3graham · 2025-04-04T12:48:01Z

/integrate

openjdk · 2025-04-04T12:49:16Z

@j3graham
Your change (at version dda134f) is now ready to be sponsored by a Committer.

wenshao · 2025-04-04T13:21:42Z

/sponsor

openjdk · 2025-04-04T13:24:23Z

Going to push as commit 37f8e41.
Since your change was applied there have been 97 commits pushed to the master branch:

4dca735: 8315844: $LSB_RELEASE is not defined before use
b92a443: 8353471: ZGC: Redundant generation id in ZGeneration
cfcb330: 8353681: jpackage suppresses errors when executed with --verbose option
10d1fec: 8353679: Restructure classes in jdk.jpackage.internal package
1161b56: 8353053: (fs) Add support for UserDefinedFileAttributeView on AIX
a13e34d: 8353274: [PPC64] Bug related to -XX:+UseCompactObjectHeaders -XX:-UseSIGTRAP in JDK-8305895
41d4a0d: 8352392: AIX: implement attach API v2 and streaming output
1c2a553: 8327858: Improve spliterator and forEach for single-element immutable collections
a449aee: 8350704: Create tests to ensure the failure behavior of core reflection APIs
57df89c: 8353684: [BACKOUT] j.u.l.Handler classes create deadlock risk via synchronized publish() method
... and 87 more: https://git.openjdk.org/jdk/compare/3d2c3cd40ebce901d09a2479c267342e04e6f79c...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-04-04T13:24:40Z

@wenshao @j3graham Pushed as commit 37f8e41.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

j3graham · 2025-04-04T15:18:04Z

Thank you all for your help. I’m looking forward to #17508 making these kinds of optimizations more systematic.

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 hotspot-compiler-dev@openjdk.org label Jan 13, 2025

j3graham mentioned this pull request Jan 13, 2025

8353741: Eliminate table lookup in UUID.toString #22928

Closed

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

add missing import

94a32db

j3graham added 2 commits March 31, 2025 22:24

address review comments

59875d5

remove unused methods

50d35dc

iwanowww reviewed Apr 1, 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

update comments

dda134f

iwanowww approved these changes Apr 2, 2025

View reviewed changes

eme64 approved these changes Apr 4, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 4, 2025

openjdk bot added the sponsor Pull request is ready to be sponsored label Apr 4, 2025

openjdk bot added the integrated Pull request has been integrated label Apr 4, 2025

openjdk bot closed this Apr 4, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Apr 4, 2025

liach mentioned this pull request Apr 10, 2025

8332249: Micro-optimize Method.hashCode #19433

Closed

3 tasks

merykitty mentioned this pull request May 5, 2025

8350896: Integer/Long.compress gets wrong type from CompressBitsNode::Value #23947

Closed

3 tasks

j3graham deleted the xor_const branch May 16, 2025 17:58

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

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

Uh oh!

Conversation

j3graham commented Jan 13, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

j3graham commented Jan 13, 2025

Uh oh!

bridgekeeper bot commented Jan 13, 2025

Uh oh!

openjdk bot commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Jan 13, 2025

Uh oh!

SirYwell commented Jan 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

liach commented Jan 14, 2025

Uh oh!

j3graham commented Jan 14, 2025

Uh oh!

mlbridge bot commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

eme64 commented Jan 22, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jan 22, 2025

Uh oh!

j3graham commented Jan 22, 2025

Uh oh!

iwanowww commented Mar 31, 2025

Uh oh!

merykitty commented Apr 1, 2025

Uh oh!

j3graham commented Apr 1, 2025

Uh oh!

iwanowww left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iwanowww left a comment

Choose a reason for hiding this comment

Uh oh!

eme64 commented Apr 2, 2025

Uh oh!

j3graham commented Apr 3, 2025

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

wenshao commented Apr 4, 2025

Uh oh!

openjdk bot commented Apr 4, 2025

Uh oh!

j3graham commented Apr 4, 2025

Uh oh!

openjdk bot commented Apr 4, 2025

Uh oh!

wenshao commented Apr 4, 2025

Uh oh!

openjdk bot commented Apr 4, 2025

Uh oh!

openjdk bot commented Apr 4, 2025

Uh oh!

j3graham commented Apr 4, 2025

Uh oh!

Reviewers

Assignees

j3graham commented Jan 13, 2025 •

edited by openjdk bot

Loading

openjdk bot commented Jan 13, 2025 •

edited

Loading

SirYwell commented Jan 14, 2025 •

edited

Loading

mlbridge bot commented Jan 15, 2025 •

edited

Loading

iwanowww left a comment •

edited

Loading