8297642: PhaseIdealLoop::only_has_infinite_loops must detect all loops that never lead to termination

[eme64]

The bug was a regression-fix for JDK-8294217. The problem is only a not-quite-correct assert. But the problem is not limited to infinite loops, as the example below shows it can happen with reducible loops.

Background:
We have an assert that checks that has_loops is true when it should be. If we have has_loops == false even though there are loops, we will not perform loop-opts in Compile::Optimize.

jdk/src/hotspot/share/opto/loopnode.cpp

Lines 4285 to 4293 in 8c472e4

	// Verify that the has_loops() flag set at parse time is consistent
	// with the just built loop tree. With infinite loops, it could be
	// that one pass of loop opts only finds infinite loops, clears the
	// has_loops() flag but adds NeverBranch nodes so the next loop opts
	// verification pass finds a non empty loop tree. When the back edge
	// is an exception edge, parsing doesn't set has_loops().
	assert(_ltree_root->_child == NULL || C->has_loops() || only_has_infinite_loops() || C->has_exception_backedge(), "parsing found no loops but there are some");
	// No loops after all
	if( !_ltree_root->_child && !_verify_only ) C->set_has_loops(false);

Generally, we want to verify, that if we just found loops (_ltree_root->_child != NULL) that has_loops == true.
There are a few cases where we do not care if we miss loop-opts:

• We only have infinite loops (only_has_infinite_loops()). Infinite loops never terminate anyway, so why make them faster? Plus, a loop is only infinite if it has no loop-exit other than a NeverBranch exit, even uncommon traps, loop-limit checks etc are exits. Thus, if a loop does anything interesting, it probably is not such a "true infinite loop". They can be more easily forced to occur by setting -XX:PerMethodTrapLimit=0.
• We have only exception edges.

Note that once we check the assert, we update has_loops. So if all loops disappeared, we avoid doing loop-opts henceforth.

Current implementation of PhaseIdealLoop::only_has_infinite_loops

jdk/src/hotspot/share/opto/loopnode.cpp

Lines 4183 to 4185 in 8c472e4

	// Goes over all children of the root of the loop tree, collects all controls for the loop and its inner loops then
	// checks whether any control is a branch out of the loop and if it is, whether it's not a NeverBranch.
	bool PhaseIdealLoop::only_has_infinite_loops() {

We check for loop exits, if there is one the loop should not be infinite.

The Problem

An infinte loop can have an inner loop, that subsequently loses its exit. It becomes its own infinite loop, and floats out of the outer loop. Where the outer loop enters into the former inner loop, we now have a loop-exit for the outer loop. The next time we run build_loop_tree and check the assert, it can fail, as PhaseIdealLoop::only_has_infinite_loops finds that new loop-exit from outer to inner loop.

Example: TestOnlyInfiniteLoops::test_simple (click on images to see them larger)

Nested infinite loop before loop-opts:

After build_loop_tree, the outer loop is detected as infinite, and NeverBranch is inserted. No loop is attached to loop-tree, as we do not attach newly discovered infinite loops. We will set has_loops == false after first loop-opts round.

During IGVN of first loop-opts round, some edges die. 88 IfTrue is dominated by 52 IfTrue (dominator info only becomes present during loop-opts). The outer loop now exits into the inner loop.

The second loop-opts round detects the former inner loop as an infinite loop, inserts NeverBranch. Once we run the assert, we see that we have has_loops == false, but PhaseIdealLoop::only_has_infinite_loops finds the former outer loop's exit.

Solution
If we ever only have infinite loops, then there will never be a way to get from any of those loops down to Root, except through a NeverBranch exit. So even if such an (outer) infinite loop ever has an exit, that exit cannot ever lead to Root, other than a NeverBranch exit. Thus, it is ok to still consider that loop as "infinite", even though it itself has an exit - that exit will never lead to termination.
Thus, I changed the PhaseIdealLoop::only_has_infinite_loops to check if any of the loops ever connect down to Root, except through NeverBranch nodes.

Alternative Fix
An alternative idea to my fix here: just replace the infinite loop with a uncommon trap, and if the infinite loop is ever hit revert back to the interpreter. If we do not care to optimize infinite loops, then why even compile them?
Advantages of that idea: No need for NeverBranch, no need for special-casing infinite loops.

I have another bug where assumptions are not true, because of infinite loops, and especially infinite loops not being attached to the loop-tree JDK-8296318

I'm looking forward to your feedback,
Emanuel

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8297642: PhaseIdealLoop::only_has_infinite_loops must detect all loops that never lead to termination

Reviewers

• Tobias Hartmann (@TobiHartmann - Reviewer) ⚠️ Review applies to 64852aa2
• Roland Westrelin (@rwestrel - Reviewer) ⚠️ Review applies to 64852aa2

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/11473/head:pull/11473
$ git checkout pull/11473

Update a local copy of the PR:
$ git checkout pull/11473
$ git pull https://git.openjdk.org/jdk pull/11473/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11473

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/11473.diff

[bridgekeeper]

👋 Welcome back epeter! 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]

@eme64 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]

Webrevs

• 01: Full - Incremental (56a7c57b)
• 00: Full (64852aa2)

[TobiHartmann]

Nice summary. Looks good to me otherwise. @rwestrel should also have a look.

[TobiHartmann]

-XX:+UnlockExperimentalVMOptions is not required in both @run, right?

[TobiHartmann]

-Xcomp implies -Xbatch:

jdk/src/hotspot/share/runtime/arguments.cpp

Lines 1445 to 1447 in 4458de9

	case _comp:
	UseInterpreter = false;
	BackgroundCompilation = false;

[openjdk]

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

8297642: PhaseIdealLoop::only_has_infinite_loops must detect all loops that never lead to termination

Reviewed-by: thartmann, roland

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

• ea108f5: 8298129: Let checkpoint event sizes grow beyond u4 limit
• 165dcdd: 8297718: Make NMT free:ing protocol more granular
• fbe7b00: 8298173: GarbageCollectionNotificationContentTest test failed: no decrease in Eden usage
• d8ef60b: 8298272: Clean up ProblemList
• 9353899: 8298175: JFR: Common timestamp for periodic events
• 94575d1: 8295116: C2: assert(dead->outcnt() == 0 && !dead->is_top()) failed: node must be dead
• 49b8622: 8290850: C2: create_new_if_for_predicate() does not clone pinned phi input nodes resulting in a broken graph
• 2f426cd: 8298375: Bad copyright header in test/jdk/java/lang/Character/Supplementary.java
• b9346e1: 8298033: Character.codePoint{At|Before}(char[], int, int) doesn't do JavaDoc-specified check
• 297bf6a: 8287397: Print top-level exception when snippet fails to read file
• ... and 134 more: https://git.openjdk.org/jdk/compare/9430f3e65c4900e121858dc111b6f20207e0694f...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]

Looks reasonable to me.

[eme64]

Thanks @rwestrel and @TobiHartmann for the help and reviews!
Sanity tested tier1 again.
/integrate

[openjdk]

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

• fc52f21: 8298255: JFR provide information about dynamization of number of compiler threads
• e555d54: 8298383: JFR: GenerateJfrFiles.java lacks copyright header
• c084431: 8298379: JFR: Some UNTIMED events only sets endTime
• ea108f5: 8298129: Let checkpoint event sizes grow beyond u4 limit
• 165dcdd: 8297718: Make NMT free:ing protocol more granular
• fbe7b00: 8298173: GarbageCollectionNotificationContentTest test failed: no decrease in Eden usage
• d8ef60b: 8298272: Clean up ProblemList
• 9353899: 8298175: JFR: Common timestamp for periodic events
• 94575d1: 8295116: C2: assert(dead->outcnt() == 0 && !dead->is_top()) failed: node must be dead
• 49b8622: 8290850: C2: create_new_if_for_predicate() does not clone pinned phi input nodes resulting in a broken graph
• ... and 137 more: https://git.openjdk.org/jdk/compare/9430f3e65c4900e121858dc111b6f20207e0694f...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit d562d3f.

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