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

8271954: C2: assert(false) failed: Bad graph detected in build_loop_late #5185

Closed
wants to merge 5 commits into from

Conversation

chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Aug 19, 2021

Since JDK-8252372, data nodes are sunk out of a loop with the help of pinned CastXX nodes to ensure that the data nodes do not float back into the loop. Such a chain of sunk data nodes is now pinned to the uncommon projection of a loop predicate in the testcase. Loop unswitching is then applied and this loop predicate is cloned down to the fast and the slow loop in PhaseIdealLoop::clone_predicates_to_unswitched_loop(). Internally, it uses PhaseIdealLoop::create_new_if_for_predicate() to create the new If node and the uncommon projection to the uncommon trap region. The UCT region has a phi node from an earlier split-if optimization. The code in create_new_if_for_predicate() updates this phi by just setting the data node of the old uncommon projection path as input for the new uncommon projection path:

use->add_req(use->in(proj_index));

It does this for both the slow and the fast loop. The graph looks like this afterwards (for method testNoDiamond()):

wrong_update

The data node 1226 AddI is used for the new uncommon projection paths of the slow loop and the fast loop while still also being used for the old uncommon projection path of 986 IfFalse. When later determining the earliest and latest control, we find that the early control is 986 IfFalse for 1226 Addl while the LCA is 376 IfTrue (dominates 1347 IfFalse, 1352 IfFalse and 986 IfFalse) which leads to the bad graph assertion failure.

An easy way to fix this is to just set the control to the LCA when cloning the predicates down in loop unswitching but this would revert the work of JDK-8252372 as the nodes could float back into the loop. Therefore, I suggest to improve create_new_if_for_predicate() to clone the sunk data node chain to the slow loop and the fast loop. Since the old predicate will die anyways once IGVN is applied after the useless predicates are eliminated in PhaseIdealLoop::eliminate_useless_predicates(), I propose to only clone the sunk data node chain for the fast loop once and reuse the old data nodes for the slow loop. I replace the old uncommon projection data input into the phi by TOP which should be fine because the control will die. The graph looks like this after applying these changes:

fixed_update

The algorithm also handles diamonds in the sunk data node chains.

Thanks,
Christian


Progress

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

Issue

  • JDK-8271954: C2: assert(false) failed: Bad graph detected in build_loop_late

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5185/head:pull/5185
$ git checkout pull/5185

Update a local copy of the PR:
$ git checkout pull/5185
$ git pull https://git.openjdk.java.net/jdk pull/5185/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5185

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5185.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Aug 19, 2021

👋 Welcome back chagedorn! 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 Aug 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Aug 19, 2021

@chhagedorn 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 Aug 19, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Aug 19, 2021

Webrevs

Copy link
Contributor

@rwestrel rwestrel left a comment

This looks good overall.

Node* clone = next->clone();
_igvn.register_new_node_with_optimizer(clone);
old_new->map(next->_idx, clone);
if (next->in(0) == uncommon_proj) {
Copy link
Contributor

@rwestrel rwestrel Aug 23, 2021

Choose a reason for hiding this comment

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

Doesn't that assume only one node in the chain has its control set? Wouldn't testing something like get_ctrl(next) != uncommon_proj be more robust?

Copy link
Member Author

@chhagedorn chhagedorn Aug 24, 2021

Choose a reason for hiding this comment

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

Nice catch! This assumption is indeed wrong. I played around with some Div nodes and the algorithm bails out in step 2 too early when comparing for next->in(0) == uncommon_proj without doing a rewiring. This leads to a bad graph again.

I think this check here in step 1 can be completely removed due to already checking with get_ctrl() below. Step 2 needs to be adapted such that we rewire the control input of a data nodes but also apply the rewiring process for such nodes with a control input within a chain. I pushed an update.

if (!in->is_Phi()) {
assert(!in->is_CFG(), "must be data node");
Node* in_clone = old_new->at(in->_idx);
if (in_clone != NULL && in_clone->_idx >= last_idx) {
Copy link
Contributor

@rwestrel rwestrel Aug 23, 2021

Choose a reason for hiding this comment

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

Doesn't in_clone != NULL implies in_clone->_idx >= last_idx? Should it be an assert?

Copy link
Member Author

@chhagedorn chhagedorn Aug 24, 2021

Choose a reason for hiding this comment

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

I'm not sure because old_new is shared in loopopts. I was concerned that I find old mappings here which I tried to exclude.

Copy link
Contributor

@rwestrel rwestrel Aug 24, 2021

Choose a reason for hiding this comment

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

Shouldn't it be reset between use i.e.:
memset( worklist.adr(), 0, worklist.Size()sizeof(Node) );
?

Copy link
Member Author

@chhagedorn chhagedorn Aug 25, 2021

Choose a reason for hiding this comment

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

Yes, I was mainly worried about visiting nodes from the current unswitching phase. But I think that cannot happen. I'll change it into an assert.

for (DUIterator_Fast jmax, j = uncommon_proj->fast_outs(jmax); j < jmax; j++) {
Node* data = uncommon_proj->fast_out(j);
if (!data->is_CFG()) {
_igvn.replace_input_of(data, 0, if_uct);
Copy link
Contributor

@rwestrel rwestrel Aug 26, 2021

Choose a reason for hiding this comment

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

don't you need to set_ctrl for data here?

Copy link
Member Author

@chhagedorn chhagedorn Aug 26, 2021

Choose a reason for hiding this comment

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

Right! I added it.

@@ -188,16 +189,45 @@ ProjNode* PhaseIdealLoop::create_new_if_for_predicate(ProjNode* cont_proj, Node*
// value as on original uncommon_proj pass.
assert(rgn->in(rgn->req() -1) == if_uct, "new edge should be last");
bool has_phi = false;
bool unswitch_clone_for_fast_loop = old_new != NULL && uncommon_proj->outcnt() > 1;
Copy link
Contributor

@rwestrel rwestrel Aug 26, 2021

Choose a reason for hiding this comment

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

Isn't it inconsistent that you compute unswitch_clone_for_fast_loop here but pass a precomputed is_slow_loop when the expression looks almost identical: why compute one locally but pass the other one as an argument? Or am I missing something?

Copy link
Member Author

@chhagedorn chhagedorn Aug 27, 2021

Choose a reason for hiding this comment

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

I want to pass a tristate: [no unswitching, fast loop, slow loop]. I initially had two bool flags passed to this function for that. But then I thought since old_new is only non-null in the fast loop case, I can use that one as "flag" to decide if we need to clone for the fast loop. I use unswitch_is_slow_loop as flag for the slow loop since old_new is NULL in this case. false and NULL is then the third state for no unswitching. But maybe it's better to introduce the second bool flag again to make it more clear.

Copy link
Contributor

@rwestrel rwestrel Aug 27, 2021

Choose a reason for hiding this comment

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

It would seem a single variable that carries the tri state would be the clearest option. That would require an enum though so not sure if it's worth it. Anyway, I'm good with the change as it is. I only found the 2 boolean variables a bit confusing.

Copy link
Member Author

@chhagedorn chhagedorn Aug 27, 2021

Choose a reason for hiding this comment

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

I agree that this is confusing. I think your suggestion with an enum is the most clean way and also improves the readability. I'll change that.

Copy link
Contributor

@rwestrel rwestrel Aug 27, 2021

Choose a reason for hiding this comment

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

Looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Aug 26, 2021

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

8271954: C2: assert(false) failed: Bad graph detected in build_loop_late

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

  • e66c8af: 8272866: java.util.random package summary contains incorrect mixing function in table
  • d1aeca1: 8272541: Incorrect overflow test in Toom-Cook branch of BigInteger multiplication
  • dfeb413: 8272964: java/nio/file/Files/InterruptCopy.java fails with java.lang.RuntimeException: Copy was not interrupted
  • a033aa5: 8273072: Avoid using += in configure
  • b92214a: 8272480: Remove Mutex::access rank
  • 596b075: 8258465: Headless build fails due to missing X11 headers on linux
  • a49a0c5: 8273062: Generation::refs_discovery_is_xxx functions are unused
  • ebd62bc: 8272846: Move some runtime/Metaspace/elastic/ tests out of tier1
  • 46684a4: 8262386: resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java timed out
  • c925c7f: 8273020: LibraryCallKit::sharpen_unsafe_type does not handle narrow oop array
  • ... and 182 more: https://git.openjdk.java.net/jdk/compare/0aca4f72ce48bc75f2c466d1cba4ad70d3c7b875...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 Aug 26, 2021
@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Aug 27, 2021

Thanks Roland for your careful review!

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Aug 30, 2021

Thanks Vladimir for your review!

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Aug 30, 2021

I'll integrate this once I'm back from my vacation.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Sep 16, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 16, 2021

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

  • 14dc517: 8273372: Remove scavenge trace message in psPromotionManager
  • 241ac89: 8273606: Zero: SPARC64 build fails with si_band type mismatch
  • 181292d: 8273801: Handle VMTYPE for "core" VM variant
  • 09ecb11: 8273806: compiler/cpuflags/TestSSE4Disabled.java should test for CPU feature explicitly
  • 99cfc16: 8273805: gc/g1/TestGCLogMessages.java test should handle non-JFR configs
  • 1c5de8b: 8273807: Zero: Drop incorrect test block from compiler/startup/NumCompilerThreadsCheck.java
  • 46af82e: 8273804: Platform.isTieredSupported should handle the no-compiler case
  • 2d13fb2: 8273803: Zero: Handle "zero" variant in CommandLineOptionTest.java
  • d4546b6: 8273526: Extend the OSContainer API pids controller with pids.current
  • 74ffe12: 8273575: memory leak in appendBootClassPath(), paths must be deallocated
  • ... and 402 more: https://git.openjdk.java.net/jdk/compare/0aca4f72ce48bc75f2c466d1cba4ad70d3c7b875...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Sep 16, 2021

@chhagedorn Pushed as commit c86e24d.

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

@chhagedorn chhagedorn deleted the JDK-8271954 branch Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-compiler integrated
3 participants