8292660: C2: blocks made unreachable by NeverBranch-to-Goto conversion are removed incorrectly

[robcasloz]

This changeset addresses three issues in the current removal of unreachable blocks after NeverBranch-to-goto conversion (introduced recently by JDK-8292285):

1. The unreachable block removal and pre-order index update loop skips the block next to the removed one, and iterates beyond the end of the block list (PhaseCFG::_blocks). Skipping blocks can lead to duplicate pre-order indices (Block::_pre_order) and/or pre-order indices greater than the size of the block list, causing problems in later transformations.

2. The outer block traversal loop iterates beyond the end of the block list whenever one or more unreachable blocks are removed.

3. Transitively unreachable blocks (such as B10 in the following example), arising in methods with multiple infinite loops, are not removed:

This changeset addresses issues 2 and 3 by decoupling NeverBranch-to-goto conversion from removal of unreachable code. Instead of removing the blocks eagerly, the removal is postponed to a later phase that works in an iterative worklist fashion, making it possible to remove transitively unreachable blocks such as B10 in the above example. Postponing removal to a later phase (where get_block(i)->_pre_order == i holds) also simplifies addressing issue 1: in the changeset, it is sufficient to iterate over the blocks that follow the removed block in the block list to decrement their _pre_order index.

Testing

• tier1-3 (windows-x64, linux-x64, linux-aarch64, and macosx-x64; release and debug mode).
• tier4-7 (linux-x64; debug mode).
• fuzzing (~1 h. on each platform).

---

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-8292660: C2: blocks made unreachable by NeverBranch-to-Goto conversion are removed incorrectly

Reviewers

• Vladimir Kozlov (@vnkozlov - Reviewer) ⚠️ Review applies to 7a95e6fc
• Roland Westrelin (@rwestrel - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 9976

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

Using diff file

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

[bridgekeeper]

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

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

• 02: Full - Incremental (93e600cc)
• 01: Full - Incremental (7a95e6fc)
• 00: Full (36498e58)

[robcasloz]

Thanks for the review, Vladimir! I have updated the changeset to postpone removal of unreachable blocks to after PhaseCFG::fixup_flow(). This makes the handling of block indices simpler and less error-prone: at this stage Block::_pre_order does not contain pre-order indices but simple block list indices, and Block::_rpo does not contain valid reverse post-order indices anymore (they are invalidated by block insertions in PhaseCFG::insert_goto_at()). I have also added a continue as suggested.

[vnkozlov]

Looks good.

[openjdk]

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

8292660: C2: blocks made unreachable by NeverBranch-to-Goto conversion are removed incorrectly

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

• 3ac91b0: 8293003: Review running time of Warn5 regression test
• e0168a0: 8288012: AArch64: unnecessary macro expansion in stubGenerator_aarch64
• 99c3ab0: 8293010: JDI ObjectReference/referringObjects/referringObjects001 fails: assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking
• 0fb9469: 8290126: Add a check in JavadocTester for "javadoc should not crash"
• 0a4d0ce: 8293121: (fs) Refactor UnixFileSystem copying into generic Unix, Linux, and BSD implementations
• 032be16: 8292066: Convert TestInputArgument.sh and TestSystemLoadAvg.sh to java version
• e393973: 8292990: Improve test coverage for XPath Axes: parent
• fa68371: 8292584: assert(cb != __null) failed: must be with -XX:-Inline
• 04d8069: 8230374: maxOutputSize, instead of javatest.maxOutputSize, should be used in TEST.properties
• 3d254d3: 8289510: Improve test coverage for XPath Axes: namespace
• ... and 157 more: https://git.openjdk.org/jdk/compare/f2f0cd86bf4dce4633f484476077fd090549780e...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.

[robcasloz]

Looks good.

Thanks for re-reviewing!

[RealFYang]

Verified that this also resolves issue: https://bugs.openjdk.org/browse/JDK-8292859

[robcasloz]

Verified that this also resolves issue: https://bugs.openjdk.org/browse/JDK-8292859

Thanks for checking, Fei.

[rwestrel]

Looks good to me.

[robcasloz]

Looks good to me.

Thanks for reviewing, Roland!

[robcasloz]

/integrate

[openjdk]

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

• 3464019: 8292899: CustomTzIDCheckDST.java testcase failed on AIX platform
• e92b9e4: 8293325: Minor improvements to macos catch_mach_exception_raise() error handling
• 767262e: 8292201: serviceability/sa/ClhsdbThreadContext.java fails with "'Thread "Common-Cleaner"' missing from stdout/stderr"
• a366e82: 7113208: Incorrect javadoc on java.net.DatagramPacket.setLength()
• ac05bc8: 8293293: Move archive heap loading code out of heapShared.cpp
• e1e6732: 8293319: [C2 cleanup] Remove unused other_path arg in Parse::adjust_map_after_if
• 5757e21: 8292385: assert(ctrl == kit.control()) failed: Control flow was added although the intrinsic bailed out
• 3993a1f: 8292067: Convert test/sun/management/jmxremote/bootstrap shell tests to java version
• 83a3408: 8293315: Add back logging for Placeholders
• b6477fd: 8293288: bootcycle build failure after JDK-8173605
• ... and 178 more: https://git.openjdk.org/jdk/compare/f2f0cd86bf4dce4633f484476077fd090549780e...master

Your commit was automatically rebased without conflicts.

[openjdk]

@robcasloz Pushed as commit 730ced9.

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