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

8265132 : C2 compilation fails with assert "missing precedence edge" #4200

Closed
wants to merge 15 commits into from
Closed

8265132 : C2 compilation fails with assert "missing precedence edge" #4200

wants to merge 15 commits into from

Conversation

@jamsheedcm
Copy link
Member

@jamsheedcm jamsheedcm commented May 26, 2021

Issue is similar to https://bugs.openjdk.java.net/browse/JDK-8261730
but happens at next https://github.com/jamsheedcm/jdk/blob/master/src/hotspot/share/opto/gcm.cpp#L830

Request for review


Progress

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

Issue

  • JDK-8265132: C2 compilation fails with assert "missing precedence edge"

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4200

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

Using diff file

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 26, 2021

👋 Welcome back jcm! 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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented May 26, 2021

⚠️ @jamsheedcm This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

Loading

@openjdk openjdk bot added the rfr label May 26, 2021
@openjdk
Copy link

@openjdk openjdk bot commented May 26, 2021

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

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 26, 2021

Loading

@jamsheedcm
Copy link
Member Author

@jamsheedcm jamsheedcm commented May 26, 2021

Adding more commits to pull request as simply silencing assert wont do as a fix. It requires other part of code to be changed as raise_LCA_mark raise_LCA_visited algorithm maintain states between calls.

Loading

@jamsheedcm jamsheedcm closed this May 26, 2021
@jamsheedcm jamsheedcm reopened this May 26, 2021
@jamsheedcm
Copy link
Member Author

@jamsheedcm jamsheedcm commented May 26, 2021

7e254c9
1a390c7
I have removed the ASSERT toggle and made the check in product mode to skip unrelated load-stores anti-dependencies insertion and related code.

Loading

@jamsheedcm
Copy link
Member Author

@jamsheedcm jamsheedcm commented May 26, 2021

On further thought resetting raise_LCA_mark raise_LCA_visited at end of insert_anti_dependence seemed better than having costly checks in product mode.
will give it a try.

Loading

@jamsheedcm
Copy link
Member Author

@jamsheedcm jamsheedcm commented May 27, 2021

I have improved the fix by clearing raise_LCA_visted list at end of insert_ant_dependences
full changes here
https://github.com/openjdk/jdk/pull/4200/files

Loading

@jamsheedcm
Copy link
Member Author

@jamsheedcm jamsheedcm commented Jun 1, 2021

Hi, request for review.

Loading

}
#endif
assert(LCA == store_null_block || store->find_edge(load) != -1,
assert(store->find_edge(load) != -1 || unrelated_load_in_store_null_block(store, load),
Copy link
Member

@dean-long dean-long Jun 17, 2021

Choose a reason for hiding this comment

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

I'm probably missing something, but the new code doesn't look equivalent to the old code, unless the bug fix is part of the refactoring. You removed the check against LCA and added a new dominates check.

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 17, 2021

Choose a reason for hiding this comment

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

it is refactoring. both issue has same property in terms of the load being used only in null block most probably by uncommon trap. gcm late scheduling schedule it in null block, later implicit null check moves another store which is unrelated to the load in null block to null check block. we don't rerun insert_anti_dependence for null_block after these move as it is not required, so we never put an anti-dependence edge load to store here. verification phase cribs about this.

as issues are same, now i check whether the load is in null block of the store.

Loading

Copy link
Member

@dean-long dean-long Jun 17, 2021

Choose a reason for hiding this comment

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

You seem to have replaced LCA == store_null_block with the equivalent of store_null_block->dominates(get_block_for_node(load)). Please explain.

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 18, 2021

Choose a reason for hiding this comment

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

LCA was schedule late location for the load after gcm schedule late and that is null block for the store.I tried improving the fix
saying if null block for the store dominates load block then ignore the case.

Loading

Copy link
Member

@dean-long dean-long Jun 18, 2021

Choose a reason for hiding this comment

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

Can you think of a case where store_null_check != NULL, and the following are not true?

LCA == store_null_block
get_block_for_node(null_block_region)->dominates(get_block_for_node(load))

It sounds like you expect LCA == store_null_block to always be true. Do you have an example where the dominates check would not be true? If both are are preconditions that are always true, then how about making them asserts?

assert(null_block_region != NULL, "null check without null region");
assert(LCA == store_null_block, "wrong LCA");
assert(get_block_for_node(null_block_region)->dominates(get_block_for_node(load)), "does not dominate");
return true;

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 18, 2021

Choose a reason for hiding this comment

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

if LCA is above null block(ie. null_check block itself or above it) means it has uses in other places . for null_check block anti dependency will be taken care in implicit null logic. above it doesn't actually matter for our considering case i guess.

i kept dominate logic as it is more broad than equating with LCA (that means even if there are multiple basic blocks in null path the assert holds good)

Loading

Copy link
Member

@dean-long dean-long Jun 18, 2021

Choose a reason for hiding this comment

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

i kept dominate logic as it is more broad than equating with LCA (that means even if there are multiple basic blocks in null path the assert holds good)

In that case, could you add a test case with multiple basic blocks in the null path? It should crash without your dominates improvement.

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 19, 2021

Choose a reason for hiding this comment

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

I didn't say there is a case already. I just said it can even take care of that. problem with using LCA is the new fix has raised LCA, may be i should use LCA_Orig in second case. but do you see problem in using dominate in assert?

Loading

Copy link
Contributor

@veresov veresov Jun 21, 2021

Choose a reason for hiding this comment

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

Looking at PhaseCFG::implicit_null_check() it seems that there can be only a single null block with a deopt in it. It looks for a very specific shape that has a deopt on the true side of a comparison with a null. Or is there something I'm missing?

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 21, 2021

Choose a reason for hiding this comment

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

I agree, will retain the earlier fix. will use LCA_Orig for the second assert.

Loading

// Cases falling under unrelated_load_in_store_null_block can make
// raise_LCA_above_marks in inconsistent state, so always reset the
// visited blocks after use.
Block_List raise_LCA_visited_list;
Copy link
Member

@dean-long dean-long Jun 17, 2021

Choose a reason for hiding this comment

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

I'm not an expert, but this sounds like a risky change to me. Could you explain the problem in more detail, and why resetting these values is safe?

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 17, 2021

Choose a reason for hiding this comment

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

AFAIU reinit is ok. it is not reinitialized as it holds unique value.
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/block.hpp#L214

Loading

Copy link
Member

@dean-long dean-long Jun 17, 2021

Choose a reason for hiding this comment

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

I took another look, and it looks safe, but I'm concerned that it will cause raise_LCA_above_marks to do more work. Don't you think there might be a performance impact? Have you measured performance?

I still don't understand what inconsistency state this tries to solve, or how it solves it.

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 18, 2021

Choose a reason for hiding this comment

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

raise_LCA_above_marks checks and marks visited bits from LCA and move upward. i am trying to be safe by clearing visited bits where we don't use calculated LCA. marking visited and not used cases only get impacted.
Only performance issue is explicit clearing logic i guess.

Inconsistency is marking visited and not using the newly computed LCA. but yes inconsistency can never happen as all the cases are computed by this time. and new store interfering the loads can seldom come. but i was trying to be safe.

Loading

Copy link
Member Author

@jamsheedcm jamsheedcm Jun 18, 2021

Choose a reason for hiding this comment

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

I will remove the logic. as it is not anticipated case.

Loading

Copy link
Member

@dean-long dean-long Jun 18, 2021

Choose a reason for hiding this comment

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

Only performance issue is explicit clearing logic i guess

The performance impact I had in mind was this short-circuit in raise_LCA_above_marks():

465 // Test and set the visited bit.
466 if (mid->raise_LCA_visited() == mark) continue; // already visited

If the visited mark is cleared, then raise_LCA_above_marks() needs to perform its work again.
Perhaps there is an assert that can guard against the inconsistency you were concerned about.

Loading

Copy link
Member

@dean-long dean-long left a comment

Please explain your changes better for those of us like me who are not experts with this code. I have added code comments where I had questions.

Loading

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Jun 25, 2021

Both cases 8261730 and 8265132 have the same code pattern:

   foo(M a, M b) {
     if (a == null) uncommon_trap();
     int v = a.val;
     if (b == null) uncommon_trap(v); // v is local we need to pass for deoptimization
     b.val = v;
     b.val = 0;
   }

First, we have optimization to fold Back-to-back stores:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/memnode.cpp#L2657
After that the only use of v is debug info in uncommon trap

Second, a and b could be the same object in general case. Loads and store are truly dependent and we have to preserve their order in such case. But it is not in our cases because a is not null and b is null based on checks in code when we go into second uncommon trap path. 8265132 case is more complicated since object is passed in array.

  foo(M a, M b) {
     if (a == null) uncommon_trap();
     if (b == null) {
       int v = a.val;
       uncommon_trap(v); // v is local we need to pass for deoptimization
     }
     // folded: b.val = v;
     b.val = 0;
  }

And after implicit_null_check optimization we got:

  foo(M a, M b) {
     if (a == null) uncommon_trap();
     if (null_check(b.val = 0)) {
       int v = a.val;
       uncommon_trap(v); // v is local we need to pass for deoptimization
     }
  }

We could move load about store's implicit null check to avoid hitting assert but it will cause performance issue because we will execute load without using its result.

Based on this the final code is correct for our cases and we need to relax asserts as we did in 8261730 case.

I need to look more on Jamsheed extra changes to see if we need them.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

I prefer version 02 to prevent must_raise_LCA in our edge case. Otherwise you affect code for store->is_Phi(). And it is more complicated.
Yes we would need to execute the check in product version too.
First, please don't do that for general (multi-blocks) case. Do it only for implicit_null_check and uncommon trap blocks.

Block* load_block = get_block_for_node(load);
Node* end = store_block->end();
if (end->is_MachNullCheck() && (end->in(1) == store) && store_block->dominates(load_block) ) {
     Node* if_true = end->find_out_with(Op_IfTrue);
     assert(if_true != NULL, "null check without null projection");
     Node* null_block_region = if_true->find_out_with(Op_Region);
     assert(null_block_region != NULL, "null check without null region");
     return get_block_for_node(null_block_region) == load_block;
}

Loading

// This function is used by insert_anti_dependences to find unrelated loads
// stores(but aliases into same) in non-null, null blocks.
// and for the same reasons it doesn't requires an anti-dependence edge.
Copy link
Contributor

@vnkozlov vnkozlov Jun 26, 2021

Choose a reason for hiding this comment

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

Confusing 3rd line in comment. And first line could be simple:

This function is used by insert_anti_dependences to find unrelated loads for stores in implicit null checks.

Loading

// block. In this case it is safe to ignore the anti-dependence, as the
// null block is only reached if 'store' tries to write to null.
Copy link
Contributor

@vnkozlov vnkozlov Jun 26, 2021

Choose a reason for hiding this comment

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

May add additional comment line:

// block. In this case it is safe to ignore the anti-dependence, as the
// null block is only reached if 'store' tries to write to null object and
// 'load' read from non-null object (there is preceding check for that)
// These objects can't be the same.

Loading

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good. But you need to file PR for JDK 17 repo and close this one.
InterfaceSuper.java test failure could be because your open and closed repos are out of sync.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jun 30, 2021

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

8265132: C2 compilation fails with assert "missing precedence edge"

Reviewed-by: iveresov

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

  • 5c08344: 8269678: Remove unimplemented and unused os::bind_to_processor()
  • 83bce94: 8268457: XML Transformer outputs Unicode supplementary character incorrectly to HTML
  • 1810b1c: 8269516: AArch64: Assembler cleanups
  • a96012a: 8261495: Shenandoah: reconsider update references memory ordering
  • 23d2996: 8269478: Shenandoah: gc/shenandoah/mxbeans tests should be more resilient
  • a6b253d: 8269416: [JVMCI] capture libjvmci crash data to a file
  • a0f32cb: 8268906: gc/g1/mixedgc/TestOldGenCollectionUsage.java assumes that GCs take 1ms minimum
  • ee0247f: 8263461: jdk/jfr/event/gc/detailed/TestEvacuationFailedEvent.java uses wrong mechanism to cause evacuation failure
  • 3ad20fc: 8269571: NMT should print total malloc bytes and invocation count
  • b969136: 8245877: assert(_value != __null) failed: resolving NULL _value in JvmtiExport::post_compiled_method_load
  • ... and 578 more: https://git.openjdk.java.net/jdk/compare/9eaa4afc99b09f4704e4d641f95104be40b9ea66...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.

Loading

@jamsheedcm
Copy link
Member Author

@jamsheedcm jamsheedcm commented Jul 1, 2021

Thank you @vnkozlov @dean-long @veresov for the reviews and help.

jdk 17 PR => openjdk/jdk17#191

Loading

@jamsheedcm jamsheedcm closed this Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants