8277882: New subnode ideal optimization: converting "c0 - (x + c1)" into "(c0 - c1) - x" #6441

CptGit · 2021-11-17T22:50:41Z

Suggest two new optimizations that can be done in SubINode::Ideal.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8277882: New subnode ideal optimization: converting "c0 - (x + c1)" into "(c0 - c1) - x"

Reviewers

Dean Long (@dean-long - Reviewer) ⚠️ Review applies to e41eb4b
Igor Veresov (@veresov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6441/head:pull/6441
$ git checkout pull/6441

Update a local copy of the PR:
$ git checkout pull/6441
$ git pull https://git.openjdk.java.net/jdk pull/6441/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6441

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6441.diff

bridgekeeper · 2021-11-17T22:51:05Z

Hi @CptGit, welcome to this OpenJDK project and thanks for contributing!

We do not recognize you as Contributor and need to ensure you have signed the Oracle Contributor Agreement (OCA). If you have not signed the OCA, please follow the instructions. Please fill in your GitHub username in the "Username" field of the application. Once you have signed the OCA, please let us know by writing /signed in a comment in this pull request.

If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please use "Add GitHub user CptGit" as summary for the issue.

If you are contributing this work on behalf of your employer and your employer has signed the OCA, please let us know by writing /covered in a comment in this pull request.

openjdk · 2021-11-17T22:53:12Z

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

CptGit · 2021-11-17T22:57:19Z

/signed

bridgekeeper · 2021-11-17T22:57:35Z

Thank you! Please allow for up to two weeks to process your OCA, although it is usually done within one to two business days. Also, please note that pull requests that are pending an OCA check will not usually be evaluated, so your patience is appreciated!

merykitty · 2021-11-18T09:57:06Z

Hi,
Regarding your first transformation, x - (0 - y) is already transformed into (x + y) - 0 which is then simplified to x + y, see line 265.
Consider merging your second transformation with the one in line 243, too.
Cheers.

merykitty · 2021-11-18T09:58:48Z

You should provide a microbenchmark to prove the effectiveness of the transformations, too.

… added for subnode.

CptGit · 2021-11-24T23:53:14Z

@merykitty Thank you for your comments. I made the merging and included both jtreg and microbenchmark tests. Please let me know if you have any other comments.

neliasso · 2021-11-26T09:56:18Z

Hi @CptGit,

You need to open a enhancement-bug at https://bugs.openjdk.java.net/ for this PR.

The title of this PR must then be changed to "bug-number: bug-title" where the title matches the title of the bug.

Regards,
Nils

CptGit · 2021-11-26T20:39:43Z

Hi @CptGit,

You need to open a enhancement-bug at https://bugs.openjdk.java.net/ for this PR.

The title of this PR must then be changed to "bug-number: bug-title" where the title matches the title of the bug.

Regards, Nils

Thank you Nils @neliasso . I don't have an account for OpenJDK. Do you know how I can get one? I've already signed OCA and been approved.

fmatte1 · 2021-11-29T03:21:07Z

Associated JBS issue - https://bugs.openjdk.java.net/browse/JDK-8277882

mlbridge · 2021-11-29T03:40:33Z

Webrevs

src/hotspot/share/opto/subnode.cpp

dean-long · 2021-11-30T01:27:36Z

I think your tests need to check more cases, especially cases that use MIN_VALUE, MAX_VALUE, or otherwise cause underflow or overflow.

dean-long · 2021-12-01T00:07:41Z

/reviewers 2

openjdk · 2021-12-01T00:08:51Z

@dean-long
The number of required reviews for this PR is now set to 2 (with at least 1 of role reviewers).

test/hotspot/jtreg/compiler/c2/TestSubIdeal.java

merykitty · 2021-12-01T13:54:19Z

I think this could be merged with the transformation on line 216 (the one converts "x - (y+c0)" into "(x-y) - c0") and doing a branch on the constness of in1 to determine the appropriate term of in2 to do the transformation.

dean-long · 2021-12-01T20:09:04Z

Testing results for 5965a8c look good.

dean-long · 2021-12-02T03:35:59Z

Do you want to make the same change to SubLNode for completeness, and adds tests for "long" like you did for "int"?
If so, then you might find the refactoring that Roland is doing in 8262341 useful.

…tests.

CptGit · 2021-12-03T04:54:09Z

Added the same transformation for "long" as well as tests. Also renamed tests to more meaningful names.

CptGit · 2021-12-03T23:16:55Z

The failure of compiler/c2/irTests/TestUnsignedComparison.java on Linux x86 was not caused by the changes in this PR and it was already fixed: #6427.

dean-long · 2021-12-07T17:05:03Z

The latest changes still look good to me.

veresov · 2021-12-08T19:48:17Z

Looks like it's failing one the tests on 32-bit

CptGit · 2021-12-08T19:49:54Z

Looks like it's failing one the tests on 32-bit

I believe the test will be passing after merging to master. Let me try to merge.

openjdk · 2021-12-08T20:20:37Z

⚠️ @CptGit the full name on your profile does not match the author name in this pull requests' HEAD commit. If this pull request gets integrated then the author name from this pull requests' HEAD commit will be used for the resulting commit. If you wish to push a new commit with a different author name, then please run the following commands in a local repository of your personal fork:

$ git checkout hotspot-opto-add-optimizations-for-subnode
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-12-08T20:20:49Z

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

8277882: New subnode ideal optimization: converting "c0 - (x + c1)" into "(c0 - c1) - x"

Reviewed-by: dlong, iveresov

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 1 new commit pushed to the master branch:

8e8fadf: 8278428: ObjectInputStream.readFully range check incorrect

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 (@dean-long, @veresov) 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).

CptGit · 2021-12-08T20:29:16Z

/integrate

openjdk · 2021-12-08T20:41:14Z

@CptGit
Your change (at version 395ee10) is now ready to be sponsored by a Committer.

veresov · 2021-12-09T00:17:35Z

/sponsor

openjdk · 2021-12-09T00:19:10Z

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

83e6a4c: 8255409: Support the new C_GetInterfaceList, C_GetInterface, and C_SessionCancel APIs in PKCS#11 v3.0
3cec700: 8278463: [test] Serialization WritePrimitive test revised for readFully test fails
03bf55f: 8277980: ObjectMethods::bootstrap throws NPE when lookup is null
3893f4f: 8278330: dump stack trace if the jvmti test nsk/jvmti/GetThreadState/thrstat002 is failed with wrong thread state
b3faecf: 8276116: C2: optimize long range checks in int counted loops
fe2ae8e: 8276904: Optional.toString() is unnecessarily expensive
6e7b7f3: 8278251: Enable "missing-explicit-ctor" check in the jdk.unsupported.desktop module
c83b781: 8278459: ProblemList javax/swing/JTree/4908142/bug4908142.java on macosx-aarch64
8e8fadf: 8278428: ObjectInputStream.readFully range check incorrect

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-09T00:19:28Z

@veresov @CptGit Pushed as commit 3524355.

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

hotspot compiler: add new optimizations in subnode.

1035664

bridgekeeper bot added the oca Needs verification of OCA signatory status label Nov 17, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Nov 17, 2021

bridgekeeper bot added the oca-verify Needs verification of OCA signatory status label Nov 17, 2021

bridgekeeper bot removed oca Needs verification of OCA signatory status oca-verify Needs verification of OCA signatory status labels Nov 24, 2021

include jtreg and microbenchmark tests for the new ideal optimization…

4ae9fbe

… added for subnode.

CptGit closed this Nov 26, 2021

CptGit reopened this Nov 26, 2021

include @bug and @summary in the jtreg test.

0d68f75

CptGit changed the title ~~hotspot compiler: add new optimizations in subnode.~~ 8277882: New subnode ideal optimization: converting "c0 - (x + c1)" into "(c0 - c1) - x" Nov 29, 2021

openjdk bot added the rfr Pull request is ready for review label Nov 29, 2021

clean.

cc56df3

dean-long suggested changes Nov 30, 2021

View reviewed changes

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

CptGit added 2 commits November 29, 2021 23:54

include more test cases.

fd304ba

add ok_to_convert to the condition.

5965a8c

merykitty reviewed Dec 1, 2021

View reviewed changes

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

address comments from code review.

c54b3e7

CptGit added 3 commits December 1, 2021 21:38

include copyright header for tests.

d9887dc

clean.

a910f0a

include the same optimization for SubLNode::Ideal and add associated …

e41eb4b

…tests.

dean-long approved these changes Dec 7, 2021

View reviewed changes

Merge master to fix the failing test on 32-bit.

395ee10

veresov approved these changes Dec 8, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 8, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Dec 8, 2021

openjdk bot closed this Dec 9, 2021

openjdk bot added integrated Pull request has been integrated and 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 9, 2021

8277882: New subnode ideal optimization: converting "c0 - (x + c1)" into "(c0 - c1) - x" #6441

8277882: New subnode ideal optimization: converting "c0 - (x + c1)" into "(c0 - c1) - x" #6441

Uh oh!

Conversation

CptGit commented Nov 17, 2021 • 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 17, 2021

Uh oh!

openjdk bot commented Nov 17, 2021

Uh oh!

CptGit commented Nov 17, 2021

Uh oh!

bridgekeeper bot commented Nov 17, 2021

Uh oh!

merykitty commented Nov 18, 2021

Uh oh!

merykitty commented Nov 18, 2021

Uh oh!

CptGit commented Nov 24, 2021

Uh oh!

neliasso commented Nov 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CptGit commented Nov 26, 2021

Uh oh!

fmatte1 commented Nov 29, 2021

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

Uh oh!

dean-long commented Nov 30, 2021

Uh oh!

dean-long commented Dec 1, 2021

Uh oh!

openjdk bot commented Dec 1, 2021

Uh oh!

Uh oh!

merykitty commented Dec 1, 2021

Uh oh!

dean-long commented Dec 1, 2021

Uh oh!

dean-long commented Dec 2, 2021

Uh oh!

CptGit commented Dec 3, 2021

Uh oh!

CptGit commented Dec 3, 2021

Uh oh!

dean-long commented Dec 7, 2021

Uh oh!

veresov commented Dec 8, 2021

Uh oh!

CptGit commented Dec 8, 2021

Uh oh!

openjdk bot commented Dec 8, 2021

Uh oh!

openjdk bot commented Dec 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CptGit commented Dec 8, 2021

Uh oh!

openjdk bot commented Dec 8, 2021

Uh oh!

veresov commented Dec 9, 2021

Uh oh!

openjdk bot commented Dec 9, 2021

Uh oh!

openjdk bot commented Dec 9, 2021

Uh oh!

Uh oh!

CptGit commented Nov 17, 2021 •

edited by openjdk bot

Loading

neliasso commented Nov 26, 2021 •

edited

Loading

mlbridge bot commented Nov 29, 2021 •

edited

Loading

openjdk bot commented Dec 8, 2021 •

edited

Loading