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

8251925: C2: RenaissanceStressTest fails with assert(!had_error): bad dominance #954

Closed
wants to merge 4 commits into from

Conversation

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Oct 30, 2020

The dominance failures start to occur after the fix for JDK-8249749 which enabled the method SWPointer::scaled_iv_plus_offset to call itself recursively and walk the graph to match more instead of stopping immediately (no recursion):
092389e#diff-8f29dd005a0f949d108687dabb7379c73dfd85cd782da453509dc9b6cb8c9f81L3789-R3812

We check in SWPointer::offset_plus_k if a node is invariant and if so then we choose it as invariant. However, we now have cases in the Renaissance benchmarks where we select an invariant that is pinned to a CastIINode between the main and pre loop. An example is shown in the attached image. 5913 SubI is found as an invariant with the improved recursive search enabled by JDK-8249749. The control of 5913 SubI (with get_ctrl) is 5298 CastII. The problem is now that we use the invariant 5913 SubI in the pre loop limit check of 5281 CountedLoopEnd (done in SuperWord::align_initial_loop_index) because we assume that since the invariant is not part of the main loop, it can float into the pre loop. But this is prevented by 5298 CastII. This results in the dominance assertion failure when checking if the earliest control of 5270 Bool in the pre loop (5297 IfTrue because of 5913 SubI that is used by 5270 Bool) dominates the LCA of 5270 Bool (the pre loop header node).

My suggestion is to improve the invariant check in SWPointer::offset_plus_k to also check if the found invariant is not dominated by the pre loop end node. Repeated testing of the RenaissanceStressTest has not resulted in any dominance failures anymore.
dominance_failure


Progress

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

Testing

Linux aarch64 Linux arm Linux ppc64le Linux s390x Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (1/1 passed) ✔️ (6/6 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8251925: C2: RenaissanceStressTest fails with assert(!had_error): bad dominance

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Oct 30, 2020

👋 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 Oct 30, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 30, 2020

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

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 30, 2020

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

I would also suggest to run locally next jtreg command and compare number of created vector nodes to make sure your changes did not affect common cases:

$ jtreg -testjdk:/my_jdk/ -va -javaoptions:'-server -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+TraceNewVectors' -J-Djavatest.maxOutputSize=1000000 compiler/c2/cr6340864/ compiler/codegen/ compiler/loopopts/superword/ compiler/vectorization >new_vects.log

$ grep "new Vector node:" new_vects.log|wc

CountedLoopEndNode* pre_end = get_pre_loop_end(main_head);
assert(lp()->is_main_loop(), "");
CountedLoopEndNode* pre_end = cached_pre_loop_end();
assert(get_pre_loop_end(lp()), "pre loop end must still be found");
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

You already have similar assert check inside cached_pre_loop_end().

@@ -911,7 +917,8 @@ bool SuperWord::ref_is_alignable(SWPointer& p) {
if (!p.has_iv()) {
return true; // no induction variable
}
CountedLoopEndNode* pre_end = get_pre_loop_end(lp()->as_CountedLoop());
CountedLoopEndNode* pre_end = cached_pre_loop_end();
assert(get_pre_loop_end(lp()), "pre loop end must still be found");
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

You already have similar assert check inside cached_pre_loop_end().

CountedLoopEndNode* cached_pre_loop_end() {
#ifdef ASSERT
Node* pre_end = get_pre_loop_end(_lp);
assert(_lp != NULL && (pre_end == NULL || pre_end == _cached_pre_loop_end) , "real CLE either not found anymore (NULL) or unchanged");
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

Check _lp != NULL should be before it use in previous line.

#ifdef ASSERT
Node* pre_end = get_pre_loop_end(_lp);
assert(_lp != NULL && (pre_end == NULL || pre_end == _cached_pre_loop_end) , "real CLE either not found anymore (NULL) or unchanged");
assert(_cached_pre_loop_end != NULL, "should be set when fetched");
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

This should be very first assert in this method.

_cached_pre_loop_end = cached_pre_loop_end;
}

int iv_stride() { return lp()->stride_con(); }
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

Can you move this method right after set_lp() as originally?

NOT_PRODUCT(_tracer.invariant_1(n, n_c);)
return !lpt()->is_member(phase()->get_loop(n_c));
}

bool SWPointer::invariant_not_dominated_by_pre_loop_end(Node* n) {
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

I think we should have only one invariant() method which does this additional check.
And have separate method is_main_loop_member() where currently we check !invariant().

Copy link
Member Author

@chhagedorn chhagedorn Nov 3, 2020

Choose a reason for hiding this comment

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

That's a good idea, fixed.

// Check that pre loop end node does not dominate n_c. If it does, then we cannot use n as invariant in the pre loop.
// This happens, for example, when n_c is a CastII node that prevents data nodes to flow above the main loop and into
// the pre loop. Use the cached version as the real pre loop end might not be found anymore with get_pre_loop_end().
return !phase()->is_dominator(_slp->cached_pre_loop_end(), n_c);
Copy link
Contributor

@vnkozlov vnkozlov Oct 30, 2020

Choose a reason for hiding this comment

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

I think it is not enough. We don't want invariant be inside pre-loop.
Invariant should be node outside original loop (before splitting and unrolling).
We should check that n_c dominates pre-loop head.
What do you think?

Copy link
Member Author

@chhagedorn chhagedorn Nov 3, 2020

Choose a reason for hiding this comment

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

I think that makes sense. I updated that. Tests still pass.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 3, 2020

@vnkozlov Thanks for your review!

$ jtreg -testjdk:/my_jdk/ -va -javaoptions:'-server -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+TraceNewVectors' -J-Djavatest.maxOutputSize=1000000 compiler/c2/cr6340864/ compiler/codegen/ compiler/loopopts/superword/ compiler/vectorization >new_vects.log

$ grep "new Vector node:" new_vects.log|wc

I ran that with the new commit - there is no difference between the fix and base without fix:
$ grep "new Vector node:" new_vects_fix.log | wc
9439 227498 1941126
$ grep "new Vector node:" new_vects_base.log | wc
9437 227521 1941437

@vnkozlov
Copy link
Contributor

@vnkozlov vnkozlov commented Nov 3, 2020

@vnkozlov Thanks for your review!

$ jtreg -testjdk:/my_jdk/ -va -javaoptions:'-server -Xbatch -XX:-TieredCompilation -XX:CICompilerCount=1 -XX:+TraceNewVectors' -J-Djavatest.maxOutputSize=1000000 compiler/c2/cr6340864/ compiler/codegen/ compiler/loopopts/superword/ compiler/vectorization >new_vects.log
$ grep "new Vector node:" new_vects.log|wc

I ran that with the new commit - there is no difference between the fix and base without fix:
$ grep "new Vector node:" new_vects_fix.log | wc
9439 227498 1941126
$ grep "new Vector node:" new_vects_base.log | wc
9437 227521 1941437

You have 2 more new vectors with fix! 9439 vs 9437
It is actually more than I tested last time: 9421

Good - no regression.

@@ -3970,20 +3986,24 @@ bool SWPointer::offset_plus_k(Node* n, bool negate) {
return true;
}
}
if (invariant(n)) {
if (!is_main_loop_member(n)) {
Copy link
Contributor

@vnkozlov vnkozlov Nov 3, 2020

Choose a reason for hiding this comment

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

Please, add comment here explaining why !is_main_loop_member() is used in this case instead of invariant().
Other places looks good.

Copy link
Member Author

@chhagedorn chhagedorn Nov 4, 2020

Choose a reason for hiding this comment

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

I updated it and also moved the following invariant() check into the if block as it can only hold if !is_main_loop_member() is true.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Updated changes looks good

@openjdk
Copy link

@openjdk openjdk bot commented Nov 6, 2020

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

8251925: C2: RenaissanceStressTest fails with assert(!had_error): bad dominance

Reviewed-by: kvn, thartmann

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

  • 5fedb69: 8250888: nsk/jvmti/scenarios/general_functions/GF08/gf08t001/TestDriver.java fails
  • 02adaa5: 8255885: Metaspace: freelist commit counter is not updated when purging
  • fa240f2: 8256594: Unexpected warning: SIGSEGV handler flags expected:SA_RESTART|SA_SIGINFO found:SA_RESTART|SA_SIGINFO
  • 4c09525: 8256108: Create implementation for NSAccessibilityElement protocol peer
  • 6813889: 8251317: Support for CLDR version 38
  • c816464: 4916923: In MetalRootPaneUI, MetalRootLayout does not correctly calculate minimumsize
  • fae68ff: 8256640: assert(!m->is_old() || ik()->is_being_redefined()) failed: old methods should not be in vtable
  • c140773: 8256692: Zero: remove obsolete block from ZeroInterpreter::native_entry
  • 080c707: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly
  • b9db002: 8256682: JDK-8202343 is incomplete
  • ... and 32 more: https://git.openjdk.java.net/jdk/compare/f504f419d3b377f0ccfd458026a2b57a9704bdff...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 Nov 6, 2020
@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 10, 2020

@vnkozlov Thanks a lot for the careful review!

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Otherwise looks good to me.

@@ -69,7 +69,9 @@ SuperWord::SuperWord(PhaseIdealLoop* phase) :
_nlist(arena(), 8, 0, NULL), // scratch list of nodes
_stk(arena(), 8, 0, NULL), // scratch stack of nodes
_lpt(NULL), // loop tree node
_lp(NULL), // LoopNode
_lp(NULL), // CountedLoopNode
_pre_loop_head(NULL), // Pre loop CountedLoopNode
Copy link
Member

@TobiHartmann TobiHartmann Nov 19, 2020

Choose a reason for hiding this comment

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

Is this field really needed? Can't we just retrieve the pre-loop head via _pre_loop_end->loopnode()?

Copy link
Member Author

@chhagedorn chhagedorn Nov 19, 2020

Choose a reason for hiding this comment

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

You're right, it is not really needed. I removed it.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 19, 2020

@TobiHartmann Thanks for your review!

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Unfortunately with forced push I can't see incremental update.
Based on last update comments changes are reasonable.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 20, 2020

@vnkozlov Thanks for reviewing again! Yes, that's unfortunate, I wasn't aware of that. I will try to do a merge next time.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 20, 2020

This was the new commit: bcab625, the other commits are identical.

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Thanks for making these changes. Looks good to me!

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 23, 2020

@TobiHartmann Thanks for your review!

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Nov 23, 2020

/integrate

@openjdk openjdk bot closed this Nov 23, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 23, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 23, 2020

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

  • 1f32c11: 8256740: ZGC: Move closures out of zOopClosure files
  • 659aec8: 8256719: C1 flags that should have expired are still present
  • e06a683: 8256497: Zero: enable G1 and Shenandoah GCs
  • 037e49c: 8256670: Zero: enable compressed oops support back
  • d46f6f5: 8256523: Streamline Java SHA2 implementation
  • 1aa90ac: 8256822: runtime/logging/RedefineClasses.java fails with "Error: VM option 'Verbose' is develop and is available only in debug version of VM."
  • edf72f0: 8256824: test/langtools/tools/javac/diags/examples/InnerClassCantHaveStatic.java has a bad copyright
  • 9a19eb6: 8254105: allow static nested declarations
  • 14de791: 8255934: JConsole 14 and greater fails to connect to older JVM
  • 86f3602: 8256806: Shenandoah: optimize shenandoah/jni/TestPinnedGarbage.java test
  • ... and 69 more: https://git.openjdk.java.net/jdk/compare/f504f419d3b377f0ccfd458026a2b57a9704bdff...master

Your commit was automatically rebased without conflicts.

Pushed as commit e4a32be.

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

openjdk-notifier bot referenced this issue Nov 23, 2020
@chhagedorn chhagedorn deleted the JDK-8251925 branch Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants