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

8261235: C1 compilation fails with assert(res->vreg_number() == index) failed: conversion check #2543

Closed
wants to merge 2 commits into from

Conversation

@chhagedorn
Copy link
Member

@chhagedorn chhagedorn commented Feb 12, 2021

The assertion is hit because we run out of virtual registers in the linear scan in C1 and do not handle it. I fixed it by applying the same bailout as in LIRGenerator::new_register().

There is also a second issue that LIR_OprDesc::vreg_max is too big. It is only used in this bailout code. OprBits::vreg_max is defined over OprBits::data_bits which uses OprBits::non_data_bits. But OprBits::non_data_bits does not consider OprBits::pointer_bits which results in a too large value for LIR_OprDesc::vreg_max and the assertion is hit because we don't bail out, yet. This needs to be fixed as well.

Thanks,
Christian


Progress

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

Issue

  • JDK-8261235: C1 compilation fails with assert(res->vreg_number() == index) failed: conversion check

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Feb 12, 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 Feb 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Feb 12, 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.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Feb 12, 2021

Webrevs

@eirbjo
Copy link
Contributor

@eirbjo eirbjo commented Feb 12, 2021

I tested this branch on my reproducer and it works like a charm:

776  302       3       org.jaxen.saxpath.base.Verifier::isXMLLetter (6130 bytes)
[...]
compilation bailout: out of virtual registers in linear scan
4717  302       3       org.jaxen.saxpath.base.Verifier::isXMLLetter (6130 bytes)   COMPILE SKIPPED: out of virtual registers in linear scan (retry at different tier)
4718  334       4       org.jaxen.saxpath.base.Verifier::isXMLLetter (6130 bytes)

Is it expected that the bailout is logged twice?

First, a log line says: "compilation bailout: out of virtual registers in linear scan", then at the end of the next line: "COMPILE SKIPPED: out of virtual registers in linear scan (retry at different tier)" ?

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Feb 15, 2021

Thanks for verifying it!

Is it expected that the bailout is logged twice?

First, a log line says: "compilation bailout: out of virtual registers in linear scan", then at the end of the next line: "COMPILE SKIPPED: out of virtual registers in linear scan (retry at different tier)" ?

Yes, that is expected. We first log the bailout alone here:

void Compilation::bailout(const char* msg) {
assert(msg != NULL, "bailout message must exist");
if (!bailed_out()) {
// keep first bailout message
if (PrintCompilation || PrintBailouts) tty->print_cr("compilation bailout: %s", msg);
_bailout_msg = msg;
}
}

And then later, we log the entire failed task with the failure reason (in this case the bailout) here:

if (PrintCompilation) {
FormatBufferResource msg = retry_message != NULL ?
FormatBufferResource("COMPILE SKIPPED: %s (%s)", failure_reason, retry_message) :
FormatBufferResource("COMPILE SKIPPED: %s", failure_reason);
task->print(tty, msg);
}

Copy link
Member

@TobiHartmann TobiHartmann left a comment

Looks good to me.

src/hotspot/share/c1/c1_LIR.hpp Outdated Show resolved Hide resolved
@openjdk
Copy link

@openjdk openjdk bot commented Feb 16, 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:

8261235: C1 compilation fails with assert(res->vreg_number() == index) failed: conversion check

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

  • 55d7bbc: 8261607: SA attach is exceeding JNI Local Refs capacity
  • 0a50688: 8241372: Several test failures due to javax.net.ssl.SSLException: Connection reset
  • 61a659f: 8260415: Remove unused class ReferenceProcessorMTProcMutator
  • 6b6f794: 8248223: KeyAgreement spec update on multi-party key exchange support
  • 8ba390d: 8261753: Test java/lang/System/OsVersionTest.java still failing on BigSur patch versions after JDK-8253702
  • 16bd7d3: 8261336: IGV: enhance default filters
  • 3f8819c: 8261501: Shenandoah: reconsider heap statistics memory ordering
  • 3cbd16d: 8259668: Make SubTasksDone use-once
  • 219b115: 8261422: Adjust problematic String.format calls in jdk/internal/util/Preconditions.java outOfBoundsMessage
  • cdc874d: 8261601: free memory in early return in Java_sun_nio_ch_sctp_SctpChannelImpl_receive0
  • ... and 105 more: https://git.openjdk.java.net/jdk/compare/deb0544ff330fadc5aac0378766bcd36c220d7e2...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 16, 2021
@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Feb 16, 2021

Thank you Tobias for your review!

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Good.

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Feb 17, 2021

Thanks Vladimir for your review!

@chhagedorn
Copy link
Member Author

@chhagedorn chhagedorn commented Feb 17, 2021

/integrate

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

@openjdk openjdk bot commented Feb 17, 2021

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

  • a930870: 8261309: Remove remaining StoreLoad barrier with UseCondCardMark for Serial/Parallel GC
  • b955f85: 8261075: Create stubRoutines.inline.hpp with SafeFetch implementation
  • d195033: 8261842: Shenandoah: cleanup ShenandoahHeapRegionSet
  • fc1d032: 8261125: Move VM_Operation to vmOperation.hpp
  • d547e1a: 8261123: Augment discussion of equivalence classes in Object.equals and comparison methods
  • 2677f6f: 8261675: ObjectValue::set_visited(bool) sets _visited false
  • e7e20d4: 8261711: Clhsdb "versioncheck true" throws NPE every time
  • 55d7bbc: 8261607: SA attach is exceeding JNI Local Refs capacity
  • 0a50688: 8241372: Several test failures due to javax.net.ssl.SSLException: Connection reset
  • 61a659f: 8260415: Remove unused class ReferenceProcessorMTProcMutator
  • ... and 112 more: https://git.openjdk.java.net/jdk/compare/deb0544ff330fadc5aac0378766bcd36c220d7e2...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8418285.

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

@chhagedorn chhagedorn deleted the JDK-8261235 branch May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants