Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8260650: test failed with "assert(false) failed: infinite loop in PhaseIterGVN::optimize" #3022

Closed
wants to merge 3 commits into from

Conversation

veresov
Copy link
Contributor

@veresov veresov commented Mar 16, 2021

The test constructs a large tree of AddINodes that add constants to a variable. The tree is completely constant-foldable. Depending of the processing order the folding process may happen in the loop in PhaseIterGVN::transform_old() instead of going through the worklist. That blows through the loop limit, which triggers assert. The solution is to make the loop limit proportional to the number of live nodes. I chose it to be K*live_nodes(), which is already the limit in PhaseIterGVN::optimize(). I also noticed the same problem in PhaseGVN::transform_no_reclaim() and adjusted the code accordingly.


Progress

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

Issue

  • JDK-8260650: test failed with "assert(false) failed: infinite loop in PhaseIterGVN::optimize"

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 16, 2021

👋 Welcome back iveresov! 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 openjdk bot added the rfr label Mar 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 16, 2021

@veresov 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.

@openjdk openjdk bot added the hotspot-compiler label Mar 16, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 16, 2021

Webrevs

}
NOT_PRODUCT( if( loop_count != 0 ) { set_progress(); } )

NOT_PRODUCT(if(loop_count != 0) { set_progress(); })
Copy link
Member

@TobiHartmann TobiHartmann Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Newline after if is missing.

Copy link
Contributor Author

@veresov veresov Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean space? I put the space in.

Copy link
Member

@TobiHartmann TobiHartmann Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Thanks!

src/hotspot/share/opto/phaseX.cpp Outdated Show resolved Hide resolved
assert(loop_count++ < K, "infinite loop in PhaseGVN::transform");
NOT_PRODUCT(loop_count++;)
#ifdef ASSERT
if (loop_count >= K * C->live_nodes()) {
Copy link
Member

@TobiHartmann TobiHartmann Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With old code, loop_count would be 0 in the first iteration (due to the postfix increment), with your changes it is 2. Is that intended? I see that you've changed < to >= but I still find it counter-intuitive that the loop_count is not the actual count. Also, it's inconsistent with PhaseIterGVN::transform where you increment after the check.

Copy link
Contributor Author

@veresov veresov Mar 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right it is a little sloppy. I will straighten this out. The intent is to count optimization steps. Since in transform_old and transform_no_reclaim, the loop is peeled to have the first apply_ideal() happen before it, I start the loop with 1. The original code had quite a few flaws, so, don't be concerned about counting the same way, the heuristic of the limit is obviously very approximate.

@veresov
Copy link
Contributor Author

@veresov veresov commented Mar 16, 2021

@TobiHartmann, I've hopefully addressed your comments, please take another look when you have a chance.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks for updating, looks good to me.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 16, 2021

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

8260650: test failed with "assert(false) failed: infinite loop in PhaseIterGVN::optimize"

Reviewed-by: thartmann, kvn

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

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.

@openjdk openjdk bot added the ready label Mar 16, 2021
src/hotspot/share/opto/phaseX.cpp Outdated Show resolved Hide resolved
src/hotspot/share/opto/phaseX.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

@veresov
Copy link
Contributor Author

@veresov veresov commented Mar 16, 2021

/integrate

@openjdk openjdk bot closed this Mar 16, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Mar 16, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 16, 2021

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

Your commit was automatically rebased without conflicts.

Pushed as commit 996079b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated
3 participants