8257498: Remove useless skeleton predicates #2075

chhagedorn · 2021-01-14T08:14:18Z

This enhancement removes useless skeleton predicates in the same way as we already remove normal useless predicates in PhaseIdealLoop::eliminate_useless_predicates().

Thanks,
Christian

Progress

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

Issue

JDK-8257498: Remove useless skeleton predicates

Reviewers

Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to 5b716bc
Tobias Hartmann (@TobiHartmann - Reviewer)

Download

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

bridgekeeper · 2021-01-14T08:15:23Z

👋 Welcome back chagedorn! 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-01-14T08:16:17Z

@chhagedorn 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-01-14T08:19:08Z

Webrevs

rwestrel · 2021-01-15T15:54:38Z

src/hotspot/share/opto/loopnode.cpp

-      useful_predicates.push(entry->in(0)->in(1)->in(1)); // good one
+
+    if (UseLoopPredicate) {
+      predicate = find_predicate_insertion_point(entry, Deoptimization::Reason_predicate);


Don't we need to do this for Deoptimization::Reason_profile_predicate as well?

You're right, we need that as well. I updated it.

rwestrel · 2021-01-19T08:23:55Z

src/hotspot/share/opto/loopPredicate.cpp

@@ -288,6 +275,34 @@ void PhaseIdealLoop::clone_skeleton_predicates_to_unswitched_loop(IdealLoopTree*
  }
 }

+// Put all skeleton predicate projections on a list, starting at 'predicate' and going up in the tree. If 'get_opaque'
+// is set, then the Opaque4 nodes of the skeleton predicates are put on the list instead of the projections.
+void PhaseIdealLoop::get_skeleton_predicates(Node* predicate, Unique_Node_List& list, bool get_opaque) {


I would remove the get_opaque parameter, populate the list with projections (the get_opaque false case) and have the caller retrieve the opaque node (predicate->in(0)->in(1)) if that's what it needs.

Okay, I updated it and removed the get_opaque flag.

rwestrel · 2021-01-19T08:23:55Z

src/hotspot/share/opto/loopnode.cpp

+    Node* n = C->skeleton_predicate_opaque4_node(idx);
+    assert(n->Opcode() == Op_Opaque4, "must be");
+    if (!useful_predicates.member(n)) { // not in the useful list
+      C->remove_skeleton_predicate_opaque4_node(idx);


Usually when nodes are kept in a global list, when a node dies, the node is removed from the list. It's the case of Compile::_predicate_opaqs for instance. So when the logic here iterates over _predicate_opaqs and removes some of them from the graph, the node is automatically removed from the _predicate_opaqs list. I don't see similar logic for skeleton Opaque4 nodes. I think it's a problem because you could end up with a node in the list that was removed from the graph (because that part of the graph is dead) and freed from memory (which could cause a crash).

That's true. I have not considered that. I updated it to remove it in the same way as we are removing other nodes as for example expensive nodes.

openjdk · 2021-01-20T12:24:46Z

@chhagedorn 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 JDK-8257498
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

rwestrel

Other than that, looks good to me.

rwestrel · 2021-01-21T08:57:07Z

src/hotspot/share/opto/loopnode.cpp

+    predicates.push(opaq);
+  }
+}
+


I didn't realize you would have to go through an extra list. So I guess what you had before with the get_opaque parameter was better.

Yes, we need an extra one. I reverted that part back again.

openjdk · 2021-01-25T09:43:22Z

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

8257498: Remove useless skeleton predicates

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

ca20c63: 8259710: Inlining trace leaks memory
09489e2: 8260338: Some fields in HaltNode is not cloned
af155fc: 8258836: JNI local refs exceed capacity getDiagnosticCommandInfo

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.

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

TobiHartmann

Looks good to me.

TobiHartmann · 2021-01-25T10:16:26Z

src/hotspot/share/opto/loopnode.cpp

     assert(n->Opcode() == Op_Opaque1, "must be");
     if (!useful_predicates.member(n)) { // not in the useful list
       _igvn.replace_node(n, n->in(1));
     }
  }
+
+  for (int i = C->skeleton_predicate_count(); i > 0; i--) {
+    const int idx = i - 1;


Could be removed for consistency with above code.

Yes, that was a left over from a previous commit where we needed idx twice. I update that.

chhagedorn · 2021-01-27T07:25:37Z

Thanks Roland and Tobias for your reviews!

chhagedorn · 2021-02-01T09:39:00Z

/integrate

openjdk · 2021-02-01T09:39:56Z

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

ab727f0: 8260591: Shenandoah: improve parallelism for concurrent thread root scans
cf94208: 8259395: Patching automatic module with additional packages re-creates module without "requires java.base"
039affc: 8260577: Unused code in AbstractCompiler after Shark compiler removal
8a9004d: 8260574: Remove parallel constructs in GenCollectedHeap::process_roots
0da9cad: 8260501: [Vector API] Improve register usage for shift operations on x86
a61ff87: 8260685: ProblemList 2 compiler/jvmci/compilerToVM tests in Xcomp configs
fcfe647: 8260462: Missing in Modality.html
67a34da: 8260630: Templatize literal_size
6b24e98: 8259008: ArithmeticException was thrown at "Monitor Cache Dump" on HSDB
69ee314: 8249867: xml declaration is not followed by a newline
... and 93 more: https://git.openjdk.java.net/jdk/compare/d825339da5b20eccb9f98afa98c123cd90fbee15...master

Your commit was automatically rebased without conflicts.

Pushed as commit aec0377.

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

8257498: Remove useless skeleton predicates

4960139

openjdk bot added the rfr label Jan 14, 2021

openjdk bot added the hotspot-compiler label Jan 14, 2021

rwestrel suggested changes Jan 15, 2021

View changes

Add handling of Deoptimization::Reason_profile_predicate

0ed11ba

rwestrel suggested changes Jan 19, 2021

View changes

Fix removal from global list and update get_skeleton_predicates

5b716bc

openjdk bot added the merge-conflict label Jan 20, 2021

rwestrel approved these changes Jan 21, 2021

View changes

chhagedorn added 2 commits Jan 22, 2021

Revert get_skeleton_predicates again

8472300

Merge master due to conflict

9109b28

openjdk bot added ready and removed merge-conflict labels Jan 25, 2021

TobiHartmann approved these changes Jan 25, 2021

View changes

Fix consistency

0e26280

TobiHartmann approved these changes Jan 25, 2021

View changes

openjdk bot closed this Feb 1, 2021

openjdk bot added integrated and removed ready rfr labels Feb 1, 2021

chhagedorn deleted the JDK-8257498 branch Feb 5, 2021

8257498: Remove useless skeleton predicates #2075

8257498: Remove useless skeleton predicates #2075

chhagedorn commented Jan 14, 2021 •

edited by openjdk[bot] bot

bridgekeeper bot commented Jan 14, 2021

openjdk bot commented Jan 14, 2021

mlbridge bot commented Jan 14, 2021 •

edited

rwestrel Jan 15, 2021

chhagedorn Jan 18, 2021

rwestrel Jan 19, 2021

chhagedorn Jan 20, 2021

rwestrel Jan 19, 2021

chhagedorn Jan 20, 2021

openjdk bot commented Jan 20, 2021

rwestrel left a comment

rwestrel Jan 21, 2021

chhagedorn Jan 22, 2021

openjdk bot commented Jan 25, 2021 •

edited

TobiHartmann left a comment

TobiHartmann Jan 25, 2021

chhagedorn Jan 25, 2021

chhagedorn commented Jan 27, 2021

chhagedorn commented Feb 1, 2021

openjdk bot commented Feb 1, 2021

8257498: Remove useless skeleton predicates #2075

8257498: Remove useless skeleton predicates #2075

Conversation

chhagedorn commented Jan 14, 2021 • edited by openjdk[bot] bot

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Jan 14, 2021

openjdk bot commented Jan 14, 2021

mlbridge bot commented Jan 14, 2021 • edited

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 20, 2021

rwestrel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Jan 25, 2021 • edited

TobiHartmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chhagedorn commented Jan 27, 2021

chhagedorn commented Feb 1, 2021

openjdk bot commented Feb 1, 2021

chhagedorn commented Jan 14, 2021 •

edited by openjdk[bot] bot

mlbridge bot commented Jan 14, 2021 •

edited

openjdk bot commented Jan 25, 2021 •

edited