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

8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload #2707

Closed
wants to merge 2 commits into from

Conversation

@rwestrel
Copy link
Contributor

@rwestrel rwestrel commented Feb 24, 2021

The assert fires because the current IfNode's condition was not
canonicalized when it's being folded with a dominating
IfNode. idealize_test() should have taken care of that because it
executed before fold_compares() but it didn't because:

Node* new_b = phase->transform( new BoolNode(b->in(1), bt.negate()) );
if( !new_b->is_Bool() ) return NULL;

caused it to bail out. new_b is a constant. This happens because of
the order in which nodes are processed by IGVN. The If's current Bool
would also constant fold but it's in the IGVN worklist and hasn't been
processed yet.

The fix I propose is to keep Aleksey's defensive fix but to check that
the Bool input is indeed about to be transformed by IGVN and that that
would cause the IfNode to be reprocessed.

I tried to write a test case but didn't succeed. The 2 If nodes come
from a tableswitch that's transformed into a series of If based on
profile data. I couldn't reproduce the profile data with a simple test
case.


Progress

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

Issue

  • JDK-8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 24, 2021

👋 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 openjdk bot added the rfr label Feb 24, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2021

@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
Copy link

@mlbridge mlbridge bot commented Feb 24, 2021

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

okay

@openjdk
Copy link

@openjdk openjdk bot commented Feb 24, 2021

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

8261914: IfNode::fold_compares_helper faces non-canonicalized bool when running JRuby JSON workload

Reviewed-by: kvn, shade

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

  • ddd550a: 8261308: C2: assert(inner->is_valid_counted_loop(T_INT) && inner->is_strip_mined()) failed: OuterStripMinedLoop should have been removed
  • 03d888f: 8261804: Remove field _processing_is_mt, calculate it instead
  • 6800ba4: 8257500: Drawing MultiResolutionImage with ImageObserver "leaks" memory
  • 65a245e: 8262329: Fix JFR parser exception messages
  • a4c2496: 8259535: ECDSA SignatureValue do not always have the specified length
  • 2515c42: 8262332: serviceability/sa/ClhsdbJhisto.java fails with Test ERROR java.lang.RuntimeException: 'ParselTongue' missing from stdout/stderr
  • 07061fc: 8256417: Exclude TestJFRWithJMX test from running with PodMan
  • c9e9189: 8262074: Consolidate the default value of MetaspaceSize
  • 05c11bc: 8262426: Change TRAPS to Thread* for find_constrained_instance_or_array_klass()
  • d06d6f5: 8262402: Make CATCH macro assert not fatal
  • ... and 65 more: https://git.openjdk.java.net/jdk/compare/a6a7e4398aae9568e39650341aa3255ab23f9961...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.

@openjdk openjdk bot added the ready label Feb 24, 2021
return false;
}
assert(this_bool->_test.is_less() && !fail->_con, "incorrect test");
Copy link
Contributor

@shipilev shipilev Feb 25, 2021

Choose a reason for hiding this comment

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

This should lead with "this test was canonicalized" comment? Missed during the move, I think.

Copy link
Contributor

@shipilev shipilev Feb 25, 2021

Choose a reason for hiding this comment

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

I also find it a bit weird to even have the assert on this path, as we tested all cases in the if-chain before, and the only path to this assert is through lt and le -- which is is_less? Maybe I am missing something, though.

Copy link
Contributor Author

@rwestrel rwestrel Feb 25, 2021

Choose a reason for hiding this comment

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

I also find it a bit weird to even have the assert on this path, as we tested all cases in the if-chain before, and the only path to this assert is through lt and le -- which is is_less? Maybe I am missing something, though.

I kept it because of the !fail->_con part of the assert. I kept the whole assert and move it around because I'm lazy, I guess.

Copy link
Contributor

@shipilev shipilev Feb 25, 2021

Choose a reason for hiding this comment

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

That's fine. Keep the comment, though?

Copy link
Contributor Author

@rwestrel rwestrel Mar 1, 2021

Choose a reason for hiding this comment

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

right

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Mar 1, 2021

@shipilev @vnkozlov Thanks for the reviews

@rwestrel
Copy link
Contributor Author

@rwestrel rwestrel commented Mar 1, 2021

/integrate

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

@openjdk openjdk bot commented Mar 1, 2021

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

  • ddd550a: 8261308: C2: assert(inner->is_valid_counted_loop(T_INT) && inner->is_strip_mined()) failed: OuterStripMinedLoop should have been removed
  • 03d888f: 8261804: Remove field _processing_is_mt, calculate it instead
  • 6800ba4: 8257500: Drawing MultiResolutionImage with ImageObserver "leaks" memory
  • 65a245e: 8262329: Fix JFR parser exception messages
  • a4c2496: 8259535: ECDSA SignatureValue do not always have the specified length
  • 2515c42: 8262332: serviceability/sa/ClhsdbJhisto.java fails with Test ERROR java.lang.RuntimeException: 'ParselTongue' missing from stdout/stderr
  • 07061fc: 8256417: Exclude TestJFRWithJMX test from running with PodMan
  • c9e9189: 8262074: Consolidate the default value of MetaspaceSize
  • 05c11bc: 8262426: Change TRAPS to Thread* for find_constrained_instance_or_array_klass()
  • d06d6f5: 8262402: Make CATCH macro assert not fatal
  • ... and 65 more: https://git.openjdk.java.net/jdk/compare/a6a7e4398aae9568e39650341aa3255ab23f9961...master

Your commit was automatically rebased without conflicts.

Pushed as commit 20c93b3.

💡 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
3 participants