8278471: Remove unreached rules in AddNode::IdealIL #6752

CptGit · 2021-12-07T23:34:06Z

Reorder optimizations in addnode so special cases appear before general cases; otherwise the special cases would be never covered.

(a - b) + (c - d) subsumes both (a - b) + (b - c) and (a - b) + (c - a). Therefore (a - b) + (b - c) and (a - b) + (c - a) have to be placed before (a - b) + (c - d) so that they can work.

Progress

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

Issue

JDK-8278471: Remove unreached rules in AddNode::IdealIL

Reviewers

Jie Fu (@DamonFool - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6752

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

Using diff file

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

…al cases.

bridgekeeper · 2021-12-07T23:34:52Z

👋 Welcome back CptGit! 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 · 2021-12-07T23:36:54Z

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

openjdk · 2021-12-08T16:46:18Z

@CptGit this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout reorder-optimizations-in-addnode-ideal
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

fmatte1 · 2021-12-09T03:33:00Z

related JBS issue - https://bugs.openjdk.java.net/browse/JDK-8278471

mlbridge · 2021-12-09T03:42:18Z

Webrevs

merykitty · 2021-12-11T16:32:54Z

We actually don't need those since (x - c) + (c - y) would be transformed into (x + c) - (c + y) which will be transformed into x - y anyway. So you can safely remove them completely.

jdk/src/hotspot/share/opto/subnode.cpp

Line 256 in 10a51fd

// Convert "(X+A) - (X+B)" into "A - B"

CptGit · 2021-12-12T19:09:27Z

We actually don't need those since (x - c) + (c - y) would be transformed into (x + c) - (c + y) which will be transformed into x - y anyway. So you can safely remove them completely.

jdk/src/hotspot/share/opto/subnode.cpp

Line 256 in 10a51fd

// Convert "(X+A) - (X+B)" into "A - B"

Thank you for pointing this out! I have two questions:

Should I just go ahead and delete these two, (a - b) + (b - c) and (a - b) + (c - a), in this pr?
I do see you are familiar with the concatenation of different optimizations. I was wondering where I am able to find more information about the order the functions are called, for example Ideal(), Value(), Identity(), or even better the entire workflow of optimization.

Thank you very much!

merykitty · 2021-12-13T10:53:27Z

Hi, I think just deleting those 2 should be okay.

Regarding idealisation, if you look into PhaseGVN::transform, it is clear that Node::Ideal is called repeatedly until there is nothing change anymore. Then Node::Value is called to do constant propagation before Node::Identity is called to get rid of redundant operations. Finally, value numbering is the transformation which deduplicates all equivalent Nodes into a single one.

jdk/src/hotspot/share/opto/phaseX.cpp

Line 828 in ccdb9f1

Node *PhaseGVN::transform( Node *n ) {

CptGit · 2021-12-14T03:01:25Z

Hi, I think just deleting those 2 should be okay.

Regarding idealisation, if you look into PhaseGVN::transform, it is clear that Node::Ideal is called repeatedly until there is nothing change anymore. Then Node::Value is called to do constant propagation before Node::Identity is called to get rid of redundant operations. Finally, value numbering is the transformation which deduplicates all equivalent Nodes into a single one.

jdk/src/hotspot/share/opto/phaseX.cpp

Line 828 in ccdb9f1

Node *PhaseGVN::transform( Node *n ) {

Thank you so much for answering both my questions!

DamonFool

LGTM

The removed rules actually never be reached before.
So removing them won't make performance any worse.

And the removed opts are already done by (AddNode::IdealIL + SubINode::Ideal).

However, it would be better to change the JBS title as something like 'Remove unreached rules in AddNode::IdealIL'
Thanks.

openjdk · 2021-12-14T08:21:20Z

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

8278471: Remove unreached rules in AddNode::IdealIL

Reviewed-by: jiefu, kvn

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

0dbe4c5: 8278131: runtime/cds/appcds/dynamicArchive/* tests failing in loom repo
e6b28e0: Merge
d3408a4: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
69b5d49: 8272064: test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java needs update for JEP 416
d6b5544: 8278607: Misc issues in foreign API javadoc
918e339: 8278233: [macos] tools/jpackage tests timeout due to /usr/bin/osascript
f48a3e8: 8278758: runtime/BootstrapMethod/BSMCalledTwice.java fails with release VMs after JDK-8262134
0cc989b: 8278744: KeyStore:getAttributes() not returning unmodifiable Set
475ec8e: 8277919: OldObjectSample event causing bloat in the class constant pool in JFR recording
32139c1: 8262134: compiler/uncommontrap/TestDeoptOOM.java failed with "guarantee(false) failed: wrong number of expression stack elements during deopt"
... and 53 more: https://git.openjdk.java.net/jdk/compare/ec0a5ac8fec75c5c3eda3d9909d2e65d610d9854...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 (@DamonFool, @vnkozlov) 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-14T15:45:13Z

Thank you for your review. @DamonFool Would you mind making the change to JBS for me? I don't have an account there, so I am not able to edit. Thanks.

DamonFool · 2021-12-14T22:56:34Z

Thank you for your review. @DamonFool Would you mind making the change to JBS for me? I don't have an account there, so I am not able to edit. Thanks.

The JBS title had been updated.
Thanks.

CptGit · 2021-12-14T23:07:16Z

/integrate

openjdk · 2021-12-14T23:08:22Z

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

DamonFool · 2021-12-14T23:15:12Z

/integrate

Hi @CptGit , you need at least two reviews for hotspot changes.
So one more review is required.

CptGit · 2021-12-14T23:19:29Z

/integrate

Hi @CptGit , you need at least two reviews for hotspot changes. So one more review is required.

It thought it can be integrated since the bot told me.Thanks for letting me know the requirement.

lgxbslgx · 2021-12-15T01:44:52Z

/reviewers 2 reviewer

openjdk · 2021-12-15T01:45:21Z

@lgxbslgx Only the author of the pull request or Reviewers are allowed to change the number of required reviewers.

lgxbslgx · 2021-12-15T01:52:18Z

@DamonFool Now we have the command reviewers to change the number of the required reviewers. Hope more developers know this.

DamonFool · 2021-12-15T02:01:46Z

@DamonFool Now we have the command reviewers to change the number of the required reviewers. Hope more developers know this.

Nice!
Got it and thanks @lgxbslgx .

/reviewers 2 reviewer

openjdk · 2021-12-15T02:02:41Z

@DamonFool Usage: /reviewers <n> [<role>] where <n> is the number of required reviewers. If role is set, the reviewers need to have that project role. If omitted, role defaults to authors.
Number of required reviewers of role reviewers cannot be decreased below 3

CptGit · 2021-12-15T02:24:28Z

/reviewers 2

openjdk · 2021-12-15T02:25:59Z

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

lgxbslgx · 2021-12-15T02:44:37Z

@DamonFool Usage: /reviewers [] where is the number of required reviewers. If role is set, the reviewers need to have that project role. If omitted, role defaults to authors.
Number of required reviewers of role reviewers cannot be decreased below 3

@DamonFool Unfortunately, it seems that you meet a bug. I filed SKARA-1288 to follow up.

vnkozlov

Before you integrate this, please, add IR framework test which checks that conversions are really happened. Especially ones you removed.

…ed already.

…g type.

CptGit · 2021-12-16T01:24:03Z

Before you integrate this, please, add IR framework test which checks that conversions are really happened. Especially ones you removed.

Thanks a lot @vnkozlov for the suggestion of IR test. I added a test and found long type not working. Then I realized "long" did not have two conversions as "int" so I added them. These IR tests helped! Thank you!

vnkozlov

Good.

CptGit · 2021-12-16T03:44:04Z

/integrate

openjdk · 2021-12-16T03:45:35Z

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

DamonFool · 2021-12-16T03:56:19Z

/sponsor

openjdk · 2021-12-16T03:57:42Z

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

0dbe4c5: 8278131: runtime/cds/appcds/dynamicArchive/* tests failing in loom repo
e6b28e0: Merge
d3408a4: 8277964: ClassCastException with no stack trace is thrown with -Xcomp in method handle invocation
69b5d49: 8272064: test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java needs update for JEP 416
d6b5544: 8278607: Misc issues in foreign API javadoc
918e339: 8278233: [macos] tools/jpackage tests timeout due to /usr/bin/osascript
f48a3e8: 8278758: runtime/BootstrapMethod/BSMCalledTwice.java fails with release VMs after JDK-8262134
0cc989b: 8278744: KeyStore:getAttributes() not returning unmodifiable Set
475ec8e: 8277919: OldObjectSample event causing bloat in the class constant pool in JFR recording
32139c1: 8262134: compiler/uncommontrap/TestDeoptOOM.java failed with "guarantee(false) failed: wrong number of expression stack elements during deopt"
... and 53 more: https://git.openjdk.java.net/jdk/compare/ec0a5ac8fec75c5c3eda3d9909d2e65d610d9854...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-12-16T03:58:01Z

@DamonFool @CptGit Pushed as commit f6fbb5a.

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

reorder optimizations in addnode so special cases appear before gener…

3fb1e7b

…al cases.

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 7, 2021

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Dec 8, 2021

CptGit force-pushed the reorder-optimizations-in-addnode-ideal branch from 78b8f67 to 3fb1e7b Compare December 8, 2021 17:03

Merge master.

4a22599

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Dec 8, 2021

CptGit changed the title ~~[bug id TBD]: Reorder optimizations in addnode~~ 8278471: Reorder optimizations in addnode Ideal Dec 9, 2021

openjdk bot added the rfr Pull request is ready for review label Dec 9, 2021

Merge master.

10a51fd

remove unnecessary optimizations.

d4d2b18

DamonFool approved these changes Dec 14, 2021

View reviewed changes

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

CptGit changed the title ~~8278471: Reorder optimizations in addnode Ideal~~ 8278471: Remove unreached rules in AddNode::IdealIL Dec 14, 2021

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

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Dec 15, 2021

vnkozlov suggested changes Dec 15, 2021

View reviewed changes

lgxbslgx mentioned this pull request Dec 15, 2021

1288: Adjust the implementation of the 'reviewers' command to match the documentation openjdk/skara#1262

Closed

2 tasks

CptGit added 2 commits December 15, 2021 19:17

Include an ir test to verify the removed optimizations really happend…

591bf3a

…ed already.

Add missing optimizations "(A+X) - (X+B)" and "(X+A) - (B+X)" for lon…

841d404

…g type.

vnkozlov approved these changes Dec 16, 2021

View reviewed changes

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

DamonFool approved these changes Dec 16, 2021

View reviewed changes

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

openjdk bot added the integrated Pull request has been integrated label Dec 16, 2021

openjdk bot closed this Dec 16, 2021

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 16, 2021

8278471: Remove unreached rules in AddNode::IdealIL #6752

8278471: Remove unreached rules in AddNode::IdealIL #6752

Uh oh!

Conversation

CptGit commented Dec 7, 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 Dec 7, 2021

Uh oh!

openjdk bot commented Dec 7, 2021

Uh oh!

openjdk bot commented Dec 8, 2021

Uh oh!

fmatte1 commented Dec 9, 2021

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

merykitty commented Dec 11, 2021

Uh oh!

CptGit commented Dec 12, 2021

Uh oh!

merykitty commented Dec 13, 2021

Uh oh!

CptGit commented Dec 14, 2021

Uh oh!

DamonFool left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

CptGit commented Dec 14, 2021

Uh oh!

DamonFool commented Dec 14, 2021

Uh oh!

CptGit commented Dec 14, 2021

Uh oh!

openjdk bot commented Dec 14, 2021

Uh oh!

DamonFool commented Dec 14, 2021

Uh oh!

CptGit commented Dec 14, 2021

Uh oh!

lgxbslgx commented Dec 15, 2021

Uh oh!

openjdk bot commented Dec 15, 2021

Uh oh!

lgxbslgx commented Dec 15, 2021

Uh oh!

DamonFool commented Dec 15, 2021

Uh oh!

openjdk bot commented Dec 15, 2021

Uh oh!

CptGit commented Dec 15, 2021

Uh oh!

openjdk bot commented Dec 15, 2021

Uh oh!

lgxbslgx commented Dec 15, 2021

Uh oh!

vnkozlov 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!

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

Uh oh!

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

CptGit commented Dec 16, 2021

Uh oh!

openjdk bot commented Dec 16, 2021

Uh oh!

DamonFool commented Dec 16, 2021

CptGit commented Dec 7, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 9, 2021 •

edited

Loading

openjdk bot commented Dec 14, 2021 •

edited

Loading

vnkozlov left a comment •

edited

Loading

CptGit commented Dec 16, 2021 •

edited

Loading