8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop

[eme64]

Context
In IdealLoopTree::do_remove_empty_loop, we detect empty loops, and break them such that IGVN can clean away the loops.

For this, we used to simply replace the tripcount Phi node with the final iv. This has two intended effects:

1. The loop-condition collapses to false, and the CountedLoopEnd node itself collapses, as the back edge is not taken.
2. The Phi may have had other users after the loop, that effectively want to use the loop exit value. Replacing the Phi with the final iv makes sure those values are still correct.

Problem
We assume that replacing the tripcount necessarily leads to Optimizations of the nodes down to the CountedLoopEnd. If this optimization is not done, we are left with a broken loop that IGVN does not clean up. Later optimizations might now find the remnants of the loop, and assume the tripcount Phi still exists - but it does not.

In this concrete example, I had an AddI node that did not do the constant folding. We had

100 AddI <x> <y>
101 AddI 100 <int:1>

Now, if <y> gets replaced with a constant <int:2>, output node 100 AddI is notified via PhaseIterGVN::add_users_to_worklist, however node 101 AddI is an output of the output - we currently do not notify it. But we would expect that it is folded to:

102 AddI <x> <int:3>

This optimization does not happen, and the propagation down does not take place, prohibiting that the loop condition is determined to be false during IGVN.

Solution
I saw two approaches to fix this issue:

1. Ensure that all required optimizations down to the CountedLoopEnd are performed. This is difficult as it is easy to miss a small pattern. Here I could simply notify outputs of outputs, if they are both AddI nodes. Probably there will eventually be other little Optimizations that were missed, and then we have to fix them all one by one.
2. Simplify the required optimization. If we already know that the loop condition is false, why not directly replace the condition of the CountedLoopEnd with int:0? We still need to replace the tripcount Phi with the final iv, to ensure exit values are correct for users after the loop. But we can simplify the code in IdealLoopTree::do_remove_empty_loop substantially, as we do not need to clone the cmp node any more.

I picked solution 2. I suggest that we still perform solution 1, in an RFE like JDK-8257197, which catches all optimizations that IGVN could have done but failed to do.

I added to the regression test.

• The regression test fails (java assert in test) if I remove the fix belonging to the original regression test:
  4fb2bb5#diff-6a59f91cb710d682247df87c75faf602f0ff9f87e2855ead1b80719704fbedffR3132-R3138
• The regression test fails (dead loop assert) before I apply my fix.
• Test passes after my fix.

I ran a large test suite.

---

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-8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop

Reviewers

• Roland Westrelin (@rwestrel - Reviewer)
• Tobias Hartmann (@TobiHartmann - Reviewer)
• Christian Hagedorn (@chhagedorn - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10393

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10393.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

• 00: Full (76899e20)

[rwestrel]

Looks good to me.

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

8293996: C2: fix and simplify IdealLoopTree::do_remove_empty_loop

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

• 14c6ac4: 8293998: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
• 02ea338: 8293887: AArch64 build failure with GCC 12 due to maybe-uninitialized warning in libfdlibm k_rem_pio2.c
• 49a7347: 8294408: Problemlist runtime/handshake/HandshakeSuspendExitTest.java
• aa48705: 8289422: Fix and re-enable vector conditional move
• 1ddc92f: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
• 1e222bc: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
• 43eff2b: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
• b88ee1e: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
• aca4276: 8294379: Missing comma after copyright year
• 1f521a1: 8225012: sanity/client/SwingSet/src/ToolTipDemoTest.java fails on Windows
• ... and 85 more: https://git.openjdk.org/jdk/compare/8082c24a0df3f4861ea391266bdfe6cdd1a77bab...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]

Nice analysis, looks good to me too.

[chhagedorn]

Nice analysis, looks good to me, too!

As we have discussed offline, I agree that we should investigate in a separate RFE if there are other cases where we miss to optimize nodes in IGVN due to not adding outputs of outputs back to the worklist. But removing the empty loop as in your fix is better and less fragile instead of relying on IGVN.

[eme64]

Thanks @rwestrel @TobiHartmann @chhagedorn for the discussions and reviews!
/integrate

[openjdk]

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

• 14c6ac4: 8293998: [PPC64] JfrGetCallTrace: assert(_pc != nullptr) failed: must have PC
• 02ea338: 8293887: AArch64 build failure with GCC 12 due to maybe-uninitialized warning in libfdlibm k_rem_pio2.c
• 49a7347: 8294408: Problemlist runtime/handshake/HandshakeSuspendExitTest.java
• aa48705: 8289422: Fix and re-enable vector conditional move
• 1ddc92f: 8294404: [BACKOUT] JDK-8294142: make test should report only executed tests
• 1e222bc: 8293462: [macos] app image signature invalid when creating DMG or PKG from post processed signed image
• 43eff2b: 8272687: Replace StringBuffer with StringBuilder in RuleBasedCollator
• b88ee1e: 6251738: Want a top-level summary page that itemizes all spec documents referenced from javadocs (OEM spec)
• aca4276: 8294379: Missing comma after copyright year
• 1f521a1: 8225012: sanity/client/SwingSet/src/ToolTipDemoTest.java fails on Windows
• ... and 85 more: https://git.openjdk.org/jdk/compare/8082c24a0df3f4861ea391266bdfe6cdd1a77bab...master

Your commit was automatically rebased without conflicts.

[openjdk]

@eme64 Pushed as commit dd51f7e.

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