8253524: C2: Refactor code that clones predicates during loop unswitching #317

rwestrel · 2020-09-23T08:30:45Z

During unswitching, PhaseIdealLoop::create_slow_version_of_loop()
calls PhaseIdealLoop::clone_predicates_to_unswitched_loop() twice, one
for each loops, to clone some predicates above each loop. That code is
fragile as it (implicitly) requires the fast loop to be processed
first. I propose calling
PhaseIdealLoop::clone_predicates_to_unswitched_loop() a single time
and have it handle both loops in a single pass.

Progress

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

Issue

JDK-8253524: C2: Refactor code that clones predicates during loop unswitching

Reviewers

Christian Hagedorn (@chhagedorn - Reviewer)
Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 5ab947e
Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 5ab947e

Download

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

bridgekeeper · 2020-09-23T08:32:22Z

👋 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 · 2020-09-23T08:33:28Z

@rwestrel 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 (add|remove) "label" command.

mlbridge · 2020-09-23T11:02:03Z

Webrevs

03: Full - Incremental (33fb2fe)
02: Full - Incremental (93f33e9bc38ff5d9475c425caa0951baacb3dea7)
01: Full - Incremental (5ab947e)
00: Full (2e26a7c)

chhagedorn

Very nice clean-up! This makes it much easier. Apart from some minor code style comments, it looks good to me!

chhagedorn · 2020-09-23T11:46:03Z

src/hotspot/share/opto/loopPredicate.cpp

-                                                    ProjNode* new_predicate_proj, bool is_slow_loop, uint idx_before_clone,
-                                                    Node_List &old_new) {
-  assert(old_predicate_proj->is_Proj(), "must be projection");
+void PhaseIdealLoop::clone_skeleton_predicates_to_unswitched_loop(IdealLoopTree *loop, const Node_List &old_new, Deoptimization::DeoptReason reason,


Asterisk: IdealLoopTree*

chhagedorn · 2020-09-23T11:49:24Z

src/hotspot/share/opto/loopnode.hpp

+  void clone_predicates_to_unswitched_loop(IdealLoopTree *loop, const Node_List &old_new, ProjNode*& iffast, ProjNode*& ifslow);
+  ProjNode* clone_predicate_to_unswitched_loop(ProjNode* predicate_proj, Node* new_entry, Deoptimization::DeoptReason reason);
+  void clone_skeleton_predicates_to_unswitched_loop(IdealLoopTree *loop, const Node_List &old_new, Deoptimization::DeoptReason reason,
+                                      ProjNode* old_predicate_proj, ProjNode* iffast, ProjNode* ifslow);
+  void check_created_predicate_for_unswitching(const Node *new_entry) const PRODUCT_RETURN;


Move the asterisks to the types.

chhagedorn · 2020-09-23T11:51:43Z

src/hotspot/share/opto/loopPredicate.cpp

 }

+#ifndef PRODUCT
+void PhaseIdealLoop::check_created_predicate_for_unswitching(const Node *new_entry) const {


Asterisk: Node*

@chhagedorn thanks for the review (and offline comments). Should be fixed now.

chhagedorn

Looks good to me!

openjdk · 2020-09-23T13:01:17Z

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

After integration, the commit message for the final commit will be:

8253524: C2: Refactor code that clones predicates during loop unswitching

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

c303fd5: 8253581: runtime/stringtable/StringTableCleaningTest.java fails on 32-bit platforms
f3ea0d3: 8252583: Clean up unused phi-to-copy degradation mechanism
4440bda: 8219586: CodeHeap State Analytics processes dead nmethods
154b8cf: 8253014: OopStorage bulk release logs entries multiple times
ba174af: 8253048: AArch64: When CallLeaf, no need to preserve callee-saved registers in caller
3320fc0: 8253372: [TESTBUG] update tests which require jvmti - hotspot
f765a7f: 8252712: move doclint to jdk.javadoc module
c21690b: 8253464: ARM32 Zero: atomic_copy64 is incorrect, breaking volatile stores
0bc01da: 8250635: MethodArityHistogram should use Compile_lock in favour of fancy checks
812b39f: 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary
... and 3 more: https://git.openjdk.java.net/jdk/compare/3fe5886baf93dbbeba7baac4dad5876eeb7f9f5b...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.

TobiHartmann

Looks good.

TobiHartmann · 2020-09-24T05:49:53Z

src/hotspot/share/opto/loopPredicate.cpp

-  }
-#endif
+void PhaseIdealLoop::clone_predicates_to_unswitched_loop(IdealLoopTree* loop, const Node_List& old_new, ProjNode*& iffast, ProjNode*& ifslow) {
+  LoopNode* head  = loop->_head->as_Loop();


Extra whitespace after head

I'll remove the white space before I integrate the change.

rwestrel · 2020-09-24T06:44:14Z

@chhagedorn @vnkozlov @TobiHartmann thanks for the review

rwestrel · 2020-09-24T06:45:34Z

/test

openjdk · 2020-09-24T06:46:04Z

@rwestrel you need to get approval to run the tests in tier1 for commits up until 93f33e9b

chhagedorn · 2020-09-24T06:57:09Z

/test approve

openjdk · 2020-09-24T07:37:47Z

A test job has been started with id: github.com-149121954-317-698149552

openjdk · 2020-09-24T08:00:39Z

@rwestrel your test job with id github.com-149121954-317-698149552 for commits up until 93f33e9b has finished.

rwestrel · 2020-09-24T08:30:38Z

/test

openjdk · 2020-09-24T08:30:51Z

@rwestrel you need to get approval to run the tests in tier1 for commits up until 33fb2fe

chhagedorn · 2020-09-24T08:37:19Z

/test approve

openjdk · 2020-09-24T08:40:30Z

A test job has been started with id: github.com-149121954-317-698199487

openjdk · 2020-09-24T09:58:43Z

@rwestrel your test job with id github.com-149121954-317-698199487 for commits up until 33fb2fe has finished.

rwestrel · 2020-09-24T09:59:59Z

/integrate

openjdk · 2020-09-24T10:00:43Z

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

c303fd5: 8253581: runtime/stringtable/StringTableCleaningTest.java fails on 32-bit platforms
f3ea0d3: 8252583: Clean up unused phi-to-copy degradation mechanism
4440bda: 8219586: CodeHeap State Analytics processes dead nmethods
154b8cf: 8253014: OopStorage bulk release logs entries multiple times
ba174af: 8253048: AArch64: When CallLeaf, no need to preserve callee-saved registers in caller
3320fc0: 8253372: [TESTBUG] update tests which require jvmti - hotspot
f765a7f: 8252712: move doclint to jdk.javadoc module
c21690b: 8253464: ARM32 Zero: atomic_copy64 is incorrect, breaking volatile stores
0bc01da: 8250635: MethodArityHistogram should use Compile_lock in favour of fancy checks
812b39f: 8252739: Deflater.setDictionary(byte[], int off, int len) ignores the starting offset for the dictionary
... and 3 more: https://git.openjdk.java.net/jdk/compare/3fe5886baf93dbbeba7baac4dad5876eeb7f9f5b...master

Your commit was automatically rebased without conflicts.

Pushed as commit b1e2f02.

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

refactoring cloning of predicates when unswitching

6243376

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Sep 23, 2020

rwestrel added 2 commits September 23, 2020 11:55

cleanup suggested by Christian

74b8bf5

remove trailing whitespaces

2e26a7c

openjdk bot added the rfr Pull request is ready for review label Sep 23, 2020

chhagedorn suggested changes Sep 23, 2020

View reviewed changes

code style

5ab947e

chhagedorn approved these changes Sep 23, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 23, 2020

vnkozlov approved these changes Sep 23, 2020

View reviewed changes

TobiHartmann approved these changes Sep 24, 2020

View reviewed changes

openjdk bot added the test-request label Sep 24, 2020

openjdk bot removed the test-request label Sep 24, 2020

extra space

33fb2fe

rwestrel force-pushed the JDK-8253524 branch from 93f33e9 to 33fb2fe Compare September 24, 2020 08:30

openjdk bot added the test-request label Sep 24, 2020

openjdk bot removed the test-request label Sep 24, 2020

chhagedorn approved these changes Sep 24, 2020

View reviewed changes

openjdk bot closed this Sep 24, 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 Sep 24, 2020

8253524: C2: Refactor code that clones predicates during loop unswitching #317

8253524: C2: Refactor code that clones predicates during loop unswitching #317

Uh oh!

Conversation

rwestrel commented Sep 23, 2020 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Download

Uh oh!

bridgekeeper bot commented Sep 23, 2020

Uh oh!

openjdk bot commented Sep 23, 2020

Uh oh!

mlbridge bot commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

chhagedorn Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

chhagedorn Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

chhagedorn Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

rwestrel Sep 23, 2020

Choose a reason for hiding this comment

Uh oh!

chhagedorn left a comment

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TobiHartmann left a comment

Choose a reason for hiding this comment

Uh oh!

TobiHartmann Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

rwestrel Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

rwestrel commented Sep 24, 2020

Uh oh!

rwestrel commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

chhagedorn commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

rwestrel commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

chhagedorn commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

rwestrel commented Sep 24, 2020

Uh oh!

openjdk bot commented Sep 24, 2020

Uh oh!

Uh oh!

rwestrel commented Sep 23, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 23, 2020 •

edited

Loading

openjdk bot commented Sep 23, 2020 •

edited

Loading