8332268: C2: Add missing optimizations for UDivI/L and UModI/L and unify the shared logic with the signed nodes #22061

theoweidmannoracle · 2024-11-13T09:45:37Z

This PR introduces

several new optimizations to unsigned division and modulo
- x % 1, x % x, x % 2^k
- x / 1, x / x, x / 2^k
- does not implement the Granlund and Montgomery algorithm, which has been implemented for signed modulo division in the past. It is unclear if a lot is to be gained by implementing this.
tests to test existing optimizations for signed division and modulo
- does not test the Granlund and Montgomery algorithm directly

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-8332268: C2: Add missing optimizations for UDivI/L and UModI/L and unify the shared logic with the signed nodes (Enhancement - P4)

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer) Review applies to 117d1f41
Emanuel Peter (@eme64 - Reviewer) Review applies to 621cf4d1
Quan Anh Mai (@merykitty - Committer) Review applies to ee7f3b35
Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22061

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-13T09:46:18Z

👋 Welcome back theoweidmannoracle! 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 · 2024-11-13T09:46:45Z

@theoweidmannoracle 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:

8332268: C2: Add missing optimizations for UDivI/L and UModI/L and unify the shared logic with the signed nodes

Reviewed-by: chagedorn, thartmann, epeter, qamai

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 234 new commits pushed to the master branch:

a21d21f: 8345609: [C1] LIR Operations with one input should be implemented as LIR_Op1
2382a2d: 8345661: Simplify page size alignment in code heap reservation
076bfa6: 8345656: Move os alignment functions out of ReservedSpace
2826838: 8345658: WB_NMTCommitMemory redundantly records an NMT tag
c9ec271: 8345800: Update copyright year to 2024 for serviceability in files where it was missed
8e0f929: 8345805: Update copyright year to 2024 for other files where it was missed
f88c1c6: 8345773: Class-File API debug printing capability
e88e793: 8343148: C2: Refactor uses of "PhaseValue::con() + PhaseIdealLoop::set_ctrl()" into separate method
1e9204f: 8345273: Fix -Wzero-as-null-pointer-constant warnings in s390 code
c40140e: 8334581: Remove no-arg constructor BasicSliderUI()
... and 224 more: https://git.openjdk.org/jdk/compare/d33ad07c32f23aee799750c9964ab26d0cbe56f4...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 (@TobiHartmann, @eme64, @merykitty, @chhagedorn) 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 · 2024-11-13T09:47:24Z

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

merykitty · 2024-11-13T11:27:57Z

This seems similar to #9947 . Feel free to take over if you are working on this as I am not working on the PR right now and I forgot which state it is in.

theoweidmannoracle · 2024-11-13T11:51:44Z

@merykitty Thanks for the pointer. If I understand correctly, your PR is more focused on improving division by applying numerical transformations, while the focus of this PR is to add basic optimizations to unsigned division and modulo (such as division by 1) and test the optimizations present for signed division. So I think your PR would indeed complement this PR very well.

mlbridge · 2024-11-13T12:01:21Z

Webrevs

TobiHartmann

Looks good to me otherwise. Nice tests!

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

src/hotspot/share/opto/divnode.cpp

eme64

Nice improvements, some comments/ questions below about testing.

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

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

src/hotspot/share/opto/divnode.cpp

…it_division

chhagedorn

Thanks for fixing the cases! Some minor variable renaming suggestions for the existing code that you moved. Otherwise, looks good to me, too.

src/hotspot/share/opto/divnode.cpp

test/hotspot/jtreg/compiler/splitif/TestSplitDivisionThroughPhi.java

src/hotspot/share/opto/divnode.cpp

…i.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

…acle/jdk into unsigned-div-opts

theoweidmannoracle · 2024-12-10T14:54:54Z

@chhagedorn Thanks for the suggestions. I renamed the variables.

openjdk · 2024-12-10T14:56:31Z

⚠️ @theoweidmannoracle 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).

src/hotspot/share/opto/divnode.cpp

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

theoweidmannoracle · 2024-12-11T12:00:47Z

/integrate

openjdk · 2024-12-11T12:03:13Z

@theoweidmannoracle This pull request has not yet been marked as ready for integration.

theoweidmannoracle · 2024-12-11T13:39:15Z

/integrate

openjdk · 2024-12-11T13:40:38Z

@theoweidmannoracle
Your change (at version 4f33cd6) is now ready to be sponsored by a Committer.

chhagedorn · 2024-12-11T14:28:50Z

/sponsor

openjdk · 2024-12-11T14:30:59Z

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

45c914c: 8343607: C2: Shenandoah crashes during barrier expansion in Continuation::enter
a21d21f: 8345609: [C1] LIR Operations with one input should be implemented as LIR_Op1
2382a2d: 8345661: Simplify page size alignment in code heap reservation
076bfa6: 8345656: Move os alignment functions out of ReservedSpace
2826838: 8345658: WB_NMTCommitMemory redundantly records an NMT tag
c9ec271: 8345800: Update copyright year to 2024 for serviceability in files where it was missed
8e0f929: 8345805: Update copyright year to 2024 for other files where it was missed
f88c1c6: 8345773: Class-File API debug printing capability
e88e793: 8343148: C2: Refactor uses of "PhaseValue::con() + PhaseIdealLoop::set_ctrl()" into separate method
1e9204f: 8345273: Fix -Wzero-as-null-pointer-constant warnings in s390 code
... and 225 more: https://git.openjdk.org/jdk/compare/d33ad07c32f23aee799750c9964ab26d0cbe56f4...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-11T14:31:08Z

@chhagedorn @theoweidmannoracle Pushed as commit d381d58.

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

theoweidmannoracle added 13 commits November 6, 2024 11:43

Implement UDivI optimizations

a654658

Add UDivL optimizations

f0bd4ad

Fix copyright

b4c421e

Test existing ModI idealization

f5008ff

Add ModL tests

23f9923

Add ModI tests

037ee66

Add long

93bfa87

Improve UModL test

1b34e31

DRY unsigned div

557480e

Add more ModI/L tests

a445687

Move is into Type

1bd2cbf

Fix code style

cae2d05

Add 2^k-1 test

13e9909

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 13, 2024

theoweidmannoracle added 2 commits November 13, 2024 11:13

Minor fixes

57a6280

Fix test comments

9f34254

Remove transform_unsigned_* and inline

117d1f4

theoweidmannoracle marked this pull request as ready for review November 13, 2024 11:56

openjdk bot added the rfr Pull request is ready for review label Nov 13, 2024

TobiHartmann approved these changes Nov 20, 2024

View reviewed changes

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

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

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

openjdk bot added the ready Pull request is ready to be integrated label Nov 20, 2024

eme64 suggested changes Nov 25, 2024

View reviewed changes

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

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

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

Resolve review comments

996baf9

openjdk bot removed the ready Pull request is ready to be integrated label Nov 25, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Dec 10, 2024

theoweidmannoracle added 2 commits December 10, 2024 09:51

Add missing UDiv/UMod cases to no_dependent_zero_check and cannot_spl…

8810dbd

…it_division

Fix space

94425ea

chhagedorn approved these changes Dec 10, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 10, 2024

Update test/hotspot/jtreg/compiler/splitif/TestSplitDivisionThroughPh…

75659ae

…i.java Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

openjdk bot removed the ready Pull request is ready to be integrated label Dec 10, 2024

theoweidmannoracle added 2 commits December 10, 2024 15:53

Rename variables

1865a29

Merge branch 'unsigned-div-opts' of https://github.com/theoweidmannor…

fe2232e

…acle/jdk into unsigned-div-opts

chhagedorn approved these changes Dec 11, 2024

View reviewed changes

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

Update src/hotspot/share/opto/divnode.cpp

4f33cd6

Co-authored-by: Christian Hagedorn <christian.hagedorn@oracle.com>

openjdk bot added the ready Pull request is ready to be integrated label Dec 11, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Dec 11, 2024

chhagedorn approved these changes Dec 11, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 11, 2024

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 11, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 11, 2024

openjdk bot closed this Dec 11, 2024

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 Dec 11, 2024

8332268: C2: Add missing optimizations for UDivI/L and UModI/L and unify the shared logic with the signed nodes #22061

8332268: C2: Add missing optimizations for UDivI/L and UModI/L and unify the shared logic with the signed nodes #22061

Uh oh!

Conversation

theoweidmannoracle commented Nov 13, 2024 • 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!

bridgekeeper bot commented Nov 13, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Nov 13, 2024

Uh oh!

merykitty commented Nov 13, 2024

Uh oh!

theoweidmannoracle commented Nov 13, 2024

Uh oh!

mlbridge bot commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eme64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theoweidmannoracle commented Dec 10, 2024

Uh oh!

openjdk bot commented Dec 10, 2024

Uh oh!

Uh oh!

theoweidmannoracle commented Dec 11, 2024

Uh oh!

openjdk bot commented Dec 11, 2024

Uh oh!

theoweidmannoracle commented Dec 11, 2024

Uh oh!

openjdk bot commented Dec 11, 2024

Uh oh!

chhagedorn commented Dec 11, 2024

Uh oh!

openjdk bot commented Dec 11, 2024

Uh oh!

openjdk bot commented Dec 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

theoweidmannoracle commented Nov 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 13, 2024 •

edited

Loading

mlbridge bot commented Nov 13, 2024 •

edited

Loading