8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations #3012

rwestrel · 2021-03-15T12:33:19Z

It's not uncommon for c2 to leave dead uses hanging from reachable
nodes. It's usually harmless but can cause confusion (such as in
valhalla JDK-8262289, not reproducible with non valhalla code AFAICT).

Most of the time, these are caused by calls to set_req() during igvn
that should be set_req_X() calls. I added a verification pass at the
end of optimization and fixed all the ones I found.

/cc hotspot-compiler

Progress

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

Issue

JDK-8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations

Reviewers

Vladimir Ivanov (@iwanowww - Reviewer) ⚠️ Review applies to f214361
Nils Eliasson (@neliasso - Reviewer) ⚠️ Review applies to f214361

Download

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

bridgekeeper · 2021-03-15T12:34:06Z

👋 Welcome back roland! 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-03-15T12:34:58Z

@rwestrel
The hotspot-compiler label was successfully added.

mlbridge · 2021-03-15T12:38:20Z

Webrevs

iwanowww

Looks good.

Changes in src/hotspot/share/opto/subtypenode.cpp don't look relevant. I assume new verification logic uncovered a problem there. Does it make sense to integrate the fix separately?

iwanowww · 2021-03-15T13:43:30Z

src/hotspot/share/opto/node.hpp

-  int replace_edge(Node* old, Node* neww);
-  int replace_edges_in_range(Node* old, Node* neww, int start, int end);
+  int replace_edge(Node* old, Node* neww, PhaseGVN* gvn = NULL);
+  int replace_edges_in_range(Node* old, Node* neww, int start, int end, PhaseGVN* gv);


right. Will fix that.

openjdk · 2021-03-15T13:51:19Z

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

8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations

Reviewed-by: vlivanov, neliasso

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

d6b5e18: 8263191: Consolidate ThreadInVMfromJavaNoAsyncException and ThreadBlockInVMWithDeadlockCheck with existing wrappers
80cdf78: 8263544: Unused argument in ConstantPoolCacheEntry::set_field()
c0176c4: 8263552: Use String.valueOf() for char-to-String conversions
fac39fe: 8263508: Remove dead code in MethodHandleImpl
7b4aefe: 8263530: sun.awt.X11.ListHelper.removeAll() should use clear()
32c7fcc: 8263490: [macos] Crash occurs on JPasswordField with activated InputMethod
8afec70: 8260931: Implement JEP 382: New macOS Rendering Pipeline
0638303: 8263497: Clean up sun.security.krb5.PrincipalName::toByteArray
ba22e6f: 8263446: Avoid unary minus over unsigned type in ObjectSynchronizer::dec_in_use_list_ceiling
b371f90: 8263504: Some OutputMachOpcodes fields are uninitialized
... and 22 more: https://git.openjdk.java.net/jdk/compare/a9b156d358b0436584a33f71abc00c9bed9d47a3...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.

rwestrel · 2021-03-15T13:56:10Z

Changes in src/hotspot/share/opto/subtypenode.cpp don't look relevant. I assume new verification logic uncovered a problem there. Does it make sense to integrate the fix separately?

In the case of subtypenode.cpp, some nodes were created and never attached to any use.

Thanks for the review!

iwanowww · 2021-03-15T14:11:19Z

In the case of subtypenode.cpp, some nodes were created and never attached to any use.

Ok, makes sense.

So, you move the code out of if (super_t->singleton() && ...) { to avoid putting subklass on the worklist?

rwestrel · 2021-03-15T14:13:31Z

So, you move the code out of if (super_t->singleton() && ...) { to avoid putting subklass on the worklist?

Right.

neliasso · 2021-03-15T14:47:58Z

src/hotspot/share/opto/addnode.cpp

-        set_req(2,x2);
-        set_req(1,x1);
-      }
+      set_req_X(2,x2,phase);


Missing space after comma in argument list. Same a bit further down to.

Thanks for the review. Should be fixed now.

rwestrel · 2021-03-16T08:19:54Z

/integrate

openjdk · 2021-03-16T08:21:22Z

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

c484d89: 8263557: Possible NULL dereference in Arena::destruct_contents()
ba35193: 8263559: Add missing initializers to VM_PopulateDumpSharedSpace
e03a594: 8262504: Some CLHSDB command cannot know they run on remote debugger
d896246: 8263420: Incorrect function name in NSAccessibilityStaticText native peer implementation
8c1112a: 8261916: gtest/GTestWrapper.java vmErrorTest.unimplemented1_vm_assert failed
1e57087: 8263392: Allow current thread to be specified in ExceptionMark
4d1c08c: 8263616: 'Deprecatd' typo in src/hotspot/share/classfile/classFileParser.cpp
0c718ab: 8262277: URLClassLoader.getResource throws undocumented IllegalArgumentException
4f1cda4: 8263387: G1GarbageCollection JFR event gets gc phase, not gc type
5ab5244: 8263514: Minor issue in JavacFileManager.SortFiles.REVERSE
... and 36 more: https://git.openjdk.java.net/jdk/compare/a9b156d358b0436584a33f71abc00c9bed9d47a3...master

Your commit was automatically rebased without conflicts.

Pushed as commit 20297a1.

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

navyxliu · 2021-03-17T22:49:38Z

hi, @rwestrel

Is Node::set_req_X() same as PhaseIterGVN::replace_input_of()?
This even supports delayed hash. can I use them interchangeably?

  // Replace ith edge of "n" with "in"
  void replace_input_of(Node* n, int i, Node* in) {
    rehash_node_delayed(n);
    n->set_req_X(i, in, this);
  }

rwestrel · 2021-03-18T09:40:46Z

hi, @rwestrel

Is Node::set_req_X() same as PhaseIterGVN::replace_input_of()?
This even supports delayed hash. can I use them interchangeably?

They are not quite the same as replace_input_of() has the rehash_node_delayed() call. When modifying an existing node, it's a requirement to call has_delete() which rehash_node_delayed() does for you. If you're in Ideal() modifying the "this" node, the hash_delete() call happened before the ideal call.

So I guess roughly: in Ideal modifying this's input -> set_req_X(), otherwise replace_input_of()

navyxliu · 2021-03-19T10:32:42Z

@rwestrel
I see. Thanks for the explanation.
verify that your patch invokes "set_req_X" in Node::ideal() and its overrides.

fix

b34d2c0

openjdk bot added rfr Pull request is ready for review hotspot-compiler hotspot-compiler-dev@openjdk.org labels Mar 15, 2021

iwanowww approved these changes Mar 15, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 15, 2021

vladimir's review

f214361

iwanowww approved these changes Mar 15, 2021

View reviewed changes

neliasso approved these changes Mar 15, 2021

View reviewed changes

Nils' review

44a1e43

openjdk bot closed this Mar 16, 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 labels Mar 16, 2021

TobiHartmann mentioned this pull request Jun 29, 2022

8284358: Unreachable loop is not removed from C2 IR, leading to a broken graph openjdk/jdk19#92

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations #3012

8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations #3012

rwestrel commented Mar 15, 2021 •

edited by openjdk bot

Loading

bridgekeeper bot commented Mar 15, 2021

openjdk bot commented Mar 15, 2021

mlbridge bot commented Mar 15, 2021 •

edited

Loading

iwanowww left a comment

iwanowww Mar 15, 2021

rwestrel Mar 15, 2021

openjdk bot commented Mar 15, 2021 •

edited

Loading

rwestrel commented Mar 15, 2021 •

edited

Loading

iwanowww commented Mar 15, 2021

rwestrel commented Mar 15, 2021

neliasso Mar 15, 2021

rwestrel Mar 15, 2021

rwestrel commented Mar 16, 2021

openjdk bot commented Mar 16, 2021

navyxliu commented Mar 17, 2021

rwestrel commented Mar 18, 2021

navyxliu commented Mar 19, 2021

8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations #3012

8263577: C2: reachable nodes shouldn't have dead uses at the end of optimizations #3012

Conversation

rwestrel commented Mar 15, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Mar 15, 2021

openjdk bot commented Mar 15, 2021

mlbridge bot commented Mar 15, 2021 • edited Loading

Webrevs

iwanowww left a comment

Choose a reason for hiding this comment

iwanowww Mar 15, 2021

Choose a reason for hiding this comment

rwestrel Mar 15, 2021

Choose a reason for hiding this comment

openjdk bot commented Mar 15, 2021 • edited Loading

rwestrel commented Mar 15, 2021 • edited Loading

iwanowww commented Mar 15, 2021

rwestrel commented Mar 15, 2021

neliasso Mar 15, 2021

Choose a reason for hiding this comment

rwestrel Mar 15, 2021

Choose a reason for hiding this comment

rwestrel commented Mar 16, 2021

openjdk bot commented Mar 16, 2021

navyxliu commented Mar 17, 2021

rwestrel commented Mar 18, 2021

navyxliu commented Mar 19, 2021

rwestrel commented Mar 15, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Mar 15, 2021 •

edited

Loading

openjdk bot commented Mar 15, 2021 •

edited

Loading

rwestrel commented Mar 15, 2021 •

edited

Loading