8282045: When loop strip mining fails, safepoints are removed from loop anyway #7513

rwestrel · 2022-02-17T10:00:32Z

I noticed that if loop strip mining fails because a safepoint is not
found right above the exit test (following partial peel for instance),
all safepoints are removed from the loop anyway. That's fixed by the
change in IdealLoopTree::counted_loop() where rather than test if loop
strip mining is enabled, the check now verifies that loop strip mining
was successful.

With that change,
compiler/c2/irTests/TestAutoVectorization2DArray.java fails. The loop
is not converted into a strip mined loop because there's no safepoint
above the exit test after partial peeling. The loop strip mining logic
is too strict when it comes to the safepoint location. Any safepoint
that dominates the exit and is in the loop as long as there's no side
effect between the safepoint and the exit can be used. The patch
implements that change as well. TestAutoVectorization2DArray.java
passes as a result.

The existing requirement to have no safepoint on the backedge is too
strict as well. If the loop has another safepoint that can be used for
strip mining, then the safepoint on the backedge can safely be
dropped. That's also implemented by the patch.

Progress

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

Issue

JDK-8282045: When loop strip mining fails, safepoints are removed from loop anyway

Reviewers

Tobias Hartmann (@TobiHartmann - Reviewer)
Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7513

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

Using diff file

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

bridgekeeper · 2022-02-17T10:00:47Z

👋 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 · 2022-02-17T10:02:51Z

@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 pull request command.

mlbridge · 2022-02-17T10:08:40Z

Webrevs

01: Full - Incremental (fa8e76d0)
00: Full (73e1ad08)

TobiHartmann

Noticed a typo in PhaseIdealLoop::find_safepoint that you might want to fix as well with this patch:
there's not side effect -> there's no side effect

TobiHartmann · 2022-02-23T09:32:58Z

src/hotspot/share/opto/loopnode.cpp

@@ -1859,6 +1849,38 @@ bool PhaseIdealLoop::is_counted_loop(Node* x, IdealLoopTree*&loop, BasicType iv_
    }
  }

+  Node *sfpt2 = NULL;


Suggested change

Node *sfpt2 = NULL;

Node* sfpt2 = NULL;

I would suggest to rename sfpt2 to sfpt and sfpt below to backedge_sfpt.

TobiHartmann · 2022-02-23T09:33:09Z

src/hotspot/share/opto/loopnode.cpp

+  }
+
+  if (x->in(LoopNode::LoopBackControl)->Opcode() == Op_SafePoint) {
+    Node *sfpt = x->in(LoopNode::LoopBackControl);


Suggested change

Node *sfpt = x->in(LoopNode::LoopBackControl);

Node* sfpt = x->in(LoopNode::LoopBackControl);

TobiHartmann · 2022-02-23T09:46:48Z

src/hotspot/share/opto/loopnode.cpp

+      // location. Give up on that loop.
+      return false;
+    }
+    if (deleteable) {


Can't you check is_deleteable_safept inline here like old code did?

rwestrel · 2022-02-28T13:16:17Z

@TobiHartmann thanks for looking at this. The new commit should take care of your comments.

TobiHartmann

Thanks, looks good to me.

TobiHartmann · 2022-03-01T13:59:31Z

Also, I executed some testing and it all passed.

openjdk · 2022-03-01T14:00:27Z

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

8282045: When loop strip mining fails, safepoints are removed from loop anyway

Reviewed-by: thartmann, chagedorn

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

341c8bd: 8267834: Refactor G1CardSetAllocator and BufferNode::Allocator to use a common base class
fcce24c: 8281811: assert(_base == Tuple) failed: Not a Tuple after JDK-8280799
a95edee: 8281472: JVM options processing silently truncates large illegal options values
44d599a: 8227369: pd_disjoint_words_atomic() needs to be atomic
eff396f: 8280713: Related to comment inheritance jdk.javadoc cleanup and refactoring
369291b: 8282444: Module finder incorrectly assumes default file system path-separator character
d4d12ad: 8282047: Enhance StringDecode/Encode microbenchmarks
22b93a3: 8282094: [REDO] Parallel: Refactor PSCardTable::scavenge_contents_parallel
d3022f8: 8282467: add extra diagnostics for JDK-8268184
e4d9fc8: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale
... and 12 more: https://git.openjdk.java.net/jdk/compare/86723d4892485651bcae735ed13545ea89b9bb22...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.

chhagedorn

Looks good to me!

rwestrel · 2022-03-01T14:41:31Z

@TobiHartmann @chhagedorn thanks for the reviews.

rwestrel · 2022-03-01T14:41:39Z

/integrate

openjdk · 2022-03-01T14:43:27Z

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

341c8bd: 8267834: Refactor G1CardSetAllocator and BufferNode::Allocator to use a common base class
fcce24c: 8281811: assert(_base == Tuple) failed: Not a Tuple after JDK-8280799
a95edee: 8281472: JVM options processing silently truncates large illegal options values
44d599a: 8227369: pd_disjoint_words_atomic() needs to be atomic
eff396f: 8280713: Related to comment inheritance jdk.javadoc cleanup and refactoring
369291b: 8282444: Module finder incorrectly assumes default file system path-separator character
d4d12ad: 8282047: Enhance StringDecode/Encode microbenchmarks
22b93a3: 8282094: [REDO] Parallel: Refactor PSCardTable::scavenge_contents_parallel
d3022f8: 8282467: add extra diagnostics for JDK-8268184
e4d9fc8: 8282023: PropertiesStoreTest and StoreReproducibilityTest jtreg failures due to en_CA locale
... and 12 more: https://git.openjdk.java.net/jdk/compare/86723d4892485651bcae735ed13545ea89b9bb22...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-03-01T14:43:54Z

@rwestrel Pushed as commit 2c5d266.

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

fix & test

bd9748f

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Feb 17, 2022

whitespaces

73e1ad0

openjdk bot added the rfr Pull request is ready for review label Feb 17, 2022

TobiHartmann reviewed Feb 23, 2022

View reviewed changes

rwestrel added 2 commits February 28, 2022 13:43

Merge branch 'master' into JDK-8282045

0145048

review

fa8e76d

TobiHartmann approved these changes Mar 1, 2022

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Mar 1, 2022

chhagedorn approved these changes Mar 1, 2022

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Mar 1, 2022

openjdk bot closed this Mar 1, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Mar 1, 2022

8282045: When loop strip mining fails, safepoints are removed from loop anyway #7513

8282045: When loop strip mining fails, safepoints are removed from loop anyway #7513

rwestrel commented Feb 17, 2022 •

edited by openjdk bot

bridgekeeper bot commented Feb 17, 2022

openjdk bot commented Feb 17, 2022

mlbridge bot commented Feb 17, 2022 •

edited

TobiHartmann left a comment •

edited

TobiHartmann Feb 23, 2022

TobiHartmann Feb 23, 2022

TobiHartmann Feb 23, 2022

TobiHartmann Feb 23, 2022

rwestrel commented Feb 28, 2022

TobiHartmann left a comment

TobiHartmann commented Mar 1, 2022

openjdk bot commented Mar 1, 2022 •

edited

chhagedorn left a comment

rwestrel commented Mar 1, 2022

rwestrel commented Mar 1, 2022

openjdk bot commented Mar 1, 2022

openjdk bot commented Mar 1, 2022

	Node *sfpt = x->in(LoopNode::LoopBackControl);
	Node* sfpt = x->in(LoopNode::LoopBackControl);

8282045: When loop strip mining fails, safepoints are removed from loop anyway #7513

8282045: When loop strip mining fails, safepoints are removed from loop anyway #7513

Conversation

rwestrel commented Feb 17, 2022 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 17, 2022

openjdk bot commented Feb 17, 2022

mlbridge bot commented Feb 17, 2022 • edited

Webrevs

TobiHartmann left a comment • edited

Choose a reason for hiding this comment

TobiHartmann Feb 23, 2022

Choose a reason for hiding this comment

TobiHartmann Feb 23, 2022

Choose a reason for hiding this comment

TobiHartmann Feb 23, 2022

Choose a reason for hiding this comment

TobiHartmann Feb 23, 2022

Choose a reason for hiding this comment

rwestrel commented Feb 28, 2022

TobiHartmann left a comment

Choose a reason for hiding this comment

TobiHartmann commented Mar 1, 2022

openjdk bot commented Mar 1, 2022 • edited

chhagedorn left a comment

Choose a reason for hiding this comment

rwestrel commented Mar 1, 2022

rwestrel commented Mar 1, 2022

openjdk bot commented Mar 1, 2022

openjdk bot commented Mar 1, 2022

rwestrel commented Feb 17, 2022 •

edited by openjdk bot

mlbridge bot commented Feb 17, 2022 •

edited

TobiHartmann left a comment •

edited

openjdk bot commented Mar 1, 2022 •

edited