8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes #1580

iwanowww · 2020-12-02T21:21:19Z

When looking up an immediate CFG node, ReplacedNodes assumes that control input is always a CFG node.
It's not always the case: for example, a projection on a non-CFG multi node (SCMemProj and LoadStore respectively).

Proposed fix is to skip projection nodes first before checking for CFG.

Testing:

hs-tier1-6 w/ -XX:+AlwaysIncrementalInlining

Progress

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

Issue

JDK-8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes

Reviewers

Nils Eliasson (@neliasso - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer)
Tobias Hartmann (@TobiHartmann - Reviewer)

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1580/head:pull/1580
$ git checkout pull/1580

bridgekeeper · 2020-12-02T21:21:42Z

👋 Welcome back vlivanov! 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 · 2020-12-02T21:22:25Z

@iwanowww 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 · 2020-12-02T21:39:01Z

Webrevs

00: Full (29733c7)

neliasso

Looks good.

TobiHartmann

Looks good.

TobiHartmann · 2020-12-04T07:24:34Z

src/hotspot/share/opto/replacednodes.cpp

+          // Skip projections, since some of the multi nodes aren't CFG (e.g., LoadStore and SCMemProj).
+          if (n->is_Proj()) {
+            n = n->in(0);
+          }


I would suggest to merge this with the if below and adjust the comment to something like:
// Skip non-CFG nodes. Also skip projections, since some of the multi nodes aren't CFG (e.g., LoadStore and SCMemProj).

I considered that, but it would turn it into a while loop. Do you think it is worth it?

Ah right. No, that's probably not worth it.

openjdk · 2020-12-04T07:35:14Z

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

8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes

Reviewed-by: neliasso, 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 57 new commits pushed to the master branch:

d08c612: 8257708: Remove redundant unmodifiableSet wrapper from already immutable set returned by Collections.singleton
49f9e57: 8257734: Extraneous output in HmacSHA3_512 constructor
f83fd4a: 8257438: Avoid adding duplicate values into extendedKeyCodesSet
37c7835: 8257673: Build fails without shenandoahgc after JDK-8257563
f0b1194: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
36209b7: 8257547: Handle multiple prereqs on the same line in deps files
c5b32b3: 8256808: com/sun/jdi/CatchAllTest.java failed with "NullPointerException: Cannot invoke "lib.jdb.Jdb.log(String)" because "this.jdb" is null"
8526947: 6508941: java.awt.Desktop.open causes VM to crash with video files sporadically
7c7facc: 8257701: Shenandoah: objArrayKlass metadata is not marked with chunked arrays
2b73f99: 8228615: Optional.empty doc should suggest using isEmpty
... and 47 more: https://git.openjdk.java.net/jdk/compare/927504e8270b4ea44c10d7faf0959b107281ddfe...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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

iwanowww · 2020-12-04T07:52:45Z

Thanks for the reviews, Nils, Vladimir, and Tobias.

iwanowww · 2020-12-04T07:52:51Z

/integrate

openjdk · 2020-12-04T07:53:44Z

@iwanowww Since your change was applied there have been 57 commits pushed to the master branch:

d08c612: 8257708: Remove redundant unmodifiableSet wrapper from already immutable set returned by Collections.singleton
49f9e57: 8257734: Extraneous output in HmacSHA3_512 constructor
f83fd4a: 8257438: Avoid adding duplicate values into extendedKeyCodesSet
37c7835: 8257673: Build fails without shenandoahgc after JDK-8257563
f0b1194: 8235784: java/lang/invoke/VarHandles/VarHandleTestByteArrayAsInt.java fails due to timeout with fastdebug bits
36209b7: 8257547: Handle multiple prereqs on the same line in deps files
c5b32b3: 8256808: com/sun/jdi/CatchAllTest.java failed with "NullPointerException: Cannot invoke "lib.jdb.Jdb.log(String)" because "this.jdb" is null"
8526947: 6508941: java.awt.Desktop.open causes VM to crash with video files sporadically
7c7facc: 8257701: Shenandoah: objArrayKlass metadata is not marked with chunked arrays
2b73f99: 8228615: Optional.empty doc should suggest using isEmpty
... and 47 more: https://git.openjdk.java.net/jdk/compare/927504e8270b4ea44c10d7faf0959b107281ddfe...master

Your commit was automatically rebased without conflicts.

Pushed as commit 4390f2c.

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

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Dec 2, 2020

ReplacedNodes doesn't handle non-CFG multi nodes well

29733c7

iwanowww force-pushed the 8257630.replacednodes branch from 1e3269e to 29733c7 Compare December 2, 2020 21:34

iwanowww changed the title ~~ReplacedNodes doesn't handle non-CFG multi nodes well~~ 8257630: ReplacedNodes doesn't handle non-CFG multi nodes Dec 2, 2020

iwanowww marked this pull request as ready for review December 2, 2020 21:35

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2020

neliasso approved these changes Dec 2, 2020

View reviewed changes

vnkozlov approved these changes Dec 2, 2020

View reviewed changes

TobiHartmann approved these changes Dec 4, 2020

View reviewed changes

iwanowww changed the title ~~8257630: ReplacedNodes doesn't handle non-CFG multi nodes~~ 8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes Dec 4, 2020

openjdk bot added the ready Pull request is ready to be integrated label Dec 4, 2020

openjdk bot closed this Dec 4, 2020

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 labels Dec 4, 2020

8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes #1580

8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes #1580

iwanowww commented Dec 2, 2020 •

edited by openjdk bot

bridgekeeper bot commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

mlbridge bot commented Dec 2, 2020

neliasso left a comment

TobiHartmann left a comment

TobiHartmann Dec 4, 2020

iwanowww Dec 4, 2020

TobiHartmann Dec 4, 2020

openjdk bot commented Dec 4, 2020

iwanowww commented Dec 4, 2020

iwanowww commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes #1580

8257630: C2: ReplacedNodes doesn't handle non-CFG multi nodes #1580

Conversation

iwanowww commented Dec 2, 2020 • edited by openjdk bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 2, 2020

openjdk bot commented Dec 2, 2020

mlbridge bot commented Dec 2, 2020

Webrevs

neliasso left a comment

Choose a reason for hiding this comment

TobiHartmann left a comment

Choose a reason for hiding this comment

TobiHartmann Dec 4, 2020

Choose a reason for hiding this comment

iwanowww Dec 4, 2020

Choose a reason for hiding this comment

TobiHartmann Dec 4, 2020

Choose a reason for hiding this comment

openjdk bot commented Dec 4, 2020

iwanowww commented Dec 4, 2020

iwanowww commented Dec 4, 2020

openjdk bot commented Dec 4, 2020

iwanowww commented Dec 2, 2020 •

edited by openjdk bot