8267982: Set the node after peephole optimization to be removed #4276

sunny868 · 2021-06-01T01:25:38Z

Hi all,

Enable peephole can cause SIGSEGV sometimes in OopFlow::build_oop_map(Node*, int, PhaseRegAlloc*, int*)+0x238 , so must set a removed-flags for the peephole-nodes , to avoid be set-live-bit() and cause crash.
Please review this trivial change.

Thanks,
Sun Guoyun

Progress

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

Issue

JDK-8267982: Set the node after peephole optimization to be removed

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4276

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

Using diff file

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

bridgekeeper · 2021-06-01T01:26:32Z

👋 Welcome back sunny868! 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-06-01T01:28:41Z

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

mlbridge · 2021-06-01T01:32:37Z

Webrevs

TobiHartmann · 2021-06-01T06:37:17Z

The PR title needs to match the JIRA bug title (including the bug number). Please also change it to something more descriptive (like the one you had previously).

sunny868 · 2021-06-21T02:21:11Z

@TobiHartmann what I need to do ?

dean-long · 2021-06-22T22:31:55Z

I think Tobias was suggesting that you change both this PR's title and the JBS issue back to "8267982: Set the node after peephole optimization to be removed"

vnkozlov

To have new flag field in base class Node is expensive (size wise). I suggest to add it to MachNode on which peephole is executed.

Otherwise the fix is correct.

src/hotspot/share/adlc/output_c.cpp

src/hotspot/share/opto/buildOopMap.cpp

sunny868 · 2021-06-24T00:59:25Z

/test tier1

openjdk · 2021-06-24T01:00:25Z

@sunny868 you need to get approval to run the tests in tier1 for commits up until b5fc0dc

sunny868 · 2021-06-24T02:57:45Z

I made a mistake operation . I will fix it right away.

sunny868 · 2021-06-24T04:52:49Z

@TobiHartmann @vnkozlov can you help me ? I made a mistake to closed this PR, but now I can not re-open this PR.
My new commit is sunny868@00a73ec .

dean-long · 2021-06-24T22:35:53Z

@sunny868 If you search for "reopen pull request after force push" you can find some helpful suggestions on how to fix this. From what I read, you need to do a another force push back to the previous PR state before it can be reopened. If all else fails, try opening a new PR.
Do we have any github/skara experts who can help with fixing this?

vnkozlov

Looks good. I have only style fix comment.
I will test changes meanwhile because I don't see it passed through GitHub testing.

vnkozlov · 2021-06-25T01:56:46Z

src/hotspot/share/opto/buildOopMap.cpp

+          if ( def->is_Mach() && def->as_Mach()->get_removed() ) {
+            //do nothing. peephole has removed the node.
+          } else {
+            if( OptoReg::is_valid(first) ) set_live_bit(tmp_live,first);
+            if( OptoReg::is_valid(second) ) set_live_bit(tmp_live,second);
+          }


Code style. Use next pattern in code you modified (body on separate line and no spaces before/after condition):

if (cond) { foo; }

We want to correct style in all places eventually as we work on code.

vnkozlov

Good.
Tier1-3 testing passed clean.

openjdk · 2021-06-25T05:57:58Z

⚠️ @sunny868 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 8267982
$ git commit -c user.name='Preferred Full Name' --allow-empty -m 'Update full name'
$ git push

openjdk · 2021-06-25T05:58:01Z

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

8267982: Set the node after peephole optimization to be removed

Reviewed-by: kvn, thartmann

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

4fbcce1: 8269886: Inaccurate error message for compressed hprof test
a96012f: 8269803: G1: remove unnecessary NoRefDiscovery
4e18ec2: 8269993: [Test]: java/net/httpclient/DigestEchoClientSSL.java contains redundant @run tags
e54585b: 8268363: AArch64: Implement string_indexof_char intrinsic in SVE
270fbcb: Merge
c812bbb: 8269929: (test) Add diagnostic info to ProceessBuilder/Basic.java for unexpected output
6000950: 8269185: Directories in /opt/runtimepackagetest and /path/to/jdk-17 are different
1f2bf1d: 8269879: [PPC64] C2: Math.rint intrinsic uses wrong rounding mode
7fcd5ca: 8266036: class file for sun.misc.Contended not found
a49b1dc: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"
... and 583 more: https://git.openjdk.java.net/jdk/compare/ce44cd6881bcbef81a840d7961a951ba586c0eae...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 (@vnkozlov, @TobiHartmann) 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).

sunny868 · 2021-06-25T06:11:09Z

/integrate

openjdk · 2021-06-25T06:12:35Z

@sunny868
Your change (at version 9be0de1) is now ready to be sponsored by a Committer.

sunny868 · 2021-06-25T06:24:37Z

@vnkozlov Could you please sponsor it for me?

TobiHartmann

Otherwise, it looks good to me.

src/hotspot/share/opto/machnode.hpp

TobiHartmann

Looks good.

vnkozlov

Update is good.

sunny868 · 2021-07-08T08:51:34Z

/integrate

openjdk · 2021-07-08T08:52:52Z

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

sunny868 · 2021-07-08T08:54:49Z

@vnkozlov @TobiHartmann Could you please sponsor it for me?

TobiHartmann · 2021-07-08T09:11:08Z

/sponsor

openjdk · 2021-07-08T09:12:43Z

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

4fbcce1: 8269886: Inaccurate error message for compressed hprof test
a96012f: 8269803: G1: remove unnecessary NoRefDiscovery
4e18ec2: 8269993: [Test]: java/net/httpclient/DigestEchoClientSSL.java contains redundant @run tags
e54585b: 8268363: AArch64: Implement string_indexof_char intrinsic in SVE
270fbcb: Merge
c812bbb: 8269929: (test) Add diagnostic info to ProceessBuilder/Basic.java for unexpected output
6000950: 8269185: Directories in /opt/runtimepackagetest and /path/to/jdk-17 are different
1f2bf1d: 8269879: [PPC64] C2: Math.rint intrinsic uses wrong rounding mode
7fcd5ca: 8266036: class file for sun.misc.Contended not found
a49b1dc: 8269772: [macos-aarch64] test compilation failed with "SocketException: No buffer space available"
... and 583 more: https://git.openjdk.java.net/jdk/compare/ce44cd6881bcbef81a840d7961a951ba586c0eae...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-07-08T09:13:02Z

@TobiHartmann @sunny868 Pushed as commit 2c883ee.

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

openjdk bot added the rfr Pull request is ready for review label Jun 1, 2021

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Jun 1, 2021

sunny868 changed the title ~~8267982: Set the node after peephole optimization to be removed~~ enable peephole cause SIGSEGV in OopFlow::build_oop_map(Node*, int, PhaseRegAlloc*, int*)+0x238 Jun 1, 2021

openjdk bot removed the rfr Pull request is ready for review label Jun 1, 2021

sunny868 changed the title ~~enable peephole cause SIGSEGV in OopFlow::build_oop_map(Node*, int, PhaseRegAlloc*, int*)+0x238~~ 8267982: Set the node after peephole optimization to be removed Jun 1, 2021

openjdk bot added the rfr Pull request is ready for review label Jun 1, 2021

sunny868 changed the title ~~8267982: Set the node after peephole optimization to be removed~~ 8267982: enable peephole cause SIGSEGV in OopFlow::build_oop_map(Node*, int, PhaseRegAlloc*, int*)+0x238 Jun 1, 2021

vnkozlov suggested changes Jun 23, 2021

View reviewed changes

sunny868 changed the title ~~8267982: enable peephole cause SIGSEGV in OopFlow::build_oop_map(Node*, int, PhaseRegAlloc*, int*)+0x238~~ 8267982: Set the node after peephole optimization to be removed Jun 23, 2021

sunny868 force-pushed the 8267982 branch from d2e5006 to e7e943d Compare June 23, 2021 04:41

vnkozlov reviewed Jun 23, 2021

View reviewed changes

src/hotspot/share/adlc/output_c.cpp Outdated Show resolved Hide resolved

vnkozlov reviewed Jun 23, 2021

View reviewed changes

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

sunny868 force-pushed the 8267982 branch from e7e943d to b5fc0dc Compare June 23, 2021 07:13

openjdk bot added the test-request label Jun 24, 2021

sunny868 closed this Jun 24, 2021

sunny868 force-pushed the 8267982 branch from b5fc0dc to ce44cd6 Compare June 24, 2021 02:41

8267982: Set the node after peephole optimization to be removed

00a73ec

sunny868 reopened this Jun 25, 2021

openjdk bot removed the rfr Pull request is ready for review label Jun 25, 2021

8267982: Set the node after peephole optimization to be removed

b9b3d3e

openjdk bot added the rfr Pull request is ready for review label Jun 25, 2021

vnkozlov reviewed Jun 25, 2021

View reviewed changes

sunny868 added 2 commits June 25, 2021 10:56

8267982: Set the node after peephole optimization to be removed

6582ee8

8267982: Set the node after peephole optimization to be removed

9be0de1

vnkozlov approved these changes Jun 25, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 25, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Jun 25, 2021

TobiHartmann suggested changes Jul 5, 2021

View reviewed changes

src/hotspot/share/opto/machnode.hpp Outdated Show resolved Hide resolved

8267982: Set the node after peephole optimization to be removed

a289641

openjdk bot removed the sponsor Pull request is ready to be sponsored label Jul 5, 2021

TobiHartmann approved these changes Jul 5, 2021

View reviewed changes

vnkozlov approved these changes Jul 5, 2021

View reviewed changes

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

openjdk bot closed this Jul 8, 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 Jul 8, 2021

8267982: Set the node after peephole optimization to be removed #4276

8267982: Set the node after peephole optimization to be removed #4276

Uh oh!

Conversation

sunny868 commented Jun 1, 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 Jun 1, 2021

Uh oh!

openjdk bot commented Jun 1, 2021

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

TobiHartmann commented Jun 1, 2021

Uh oh!

sunny868 commented Jun 21, 2021

Uh oh!

dean-long commented Jun 22, 2021

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sunny868 commented Jun 24, 2021

Uh oh!

openjdk bot commented Jun 24, 2021

Uh oh!

sunny868 commented Jun 24, 2021

Uh oh!

sunny868 commented Jun 24, 2021

Uh oh!

dean-long commented Jun 24, 2021

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

sunny868 Jun 25, 2021

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Jun 25, 2021

Uh oh!

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

Uh oh!

Uh oh!

sunny868 commented Jun 25, 2021

Uh oh!

openjdk bot commented Jun 25, 2021

Uh oh!

sunny868 commented Jun 25, 2021

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

sunny868 commented Jul 8, 2021

Uh oh!

openjdk bot commented Jul 8, 2021

Uh oh!

sunny868 commented Jul 8, 2021

Uh oh!

TobiHartmann commented Jul 8, 2021

sunny868 commented Jun 1, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Jun 1, 2021 •

edited

Loading

openjdk bot commented Jun 25, 2021 •

edited

Loading