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

[Christian Hagedorn-3]

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

-------------

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

Changes: https://git.openjdk.java.net/jdk/pull/2543/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2543&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8261235
  Stats: 4151 lines in 6 files changed: 4137 ins; 2 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2543.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2543/head:pull/2543

PR: https://git.openjdk.java.net/jdk/pull/2543

[Eirik Bjorsnos]

On Fri, 12 Feb 2021 10:03:25 GMT, Christian Hagedorn <[hidden email]> wrote:

> 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

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)" ?

-------------

PR: https://git.openjdk.java.net/jdk/pull/2543

[Christian Hagedorn-3]

On Fri, 12 Feb 2021 17:15:01 GMT, Eirik Bjorsnos <[hidden email]> wrote:

>> 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
>
> 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)" ?

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:
https://github.com/openjdk/jdk/blob/3882fda83bebf4a8c8100fd59c37f04610491ce7/src/hotspot/share/c1/c1_Compilation.cpp#L620-L627

And then later, we log the entire failed task with the failure reason (in this case the bailout) here:
https://github.com/openjdk/jdk/blob/3882fda83bebf4a8c8100fd59c37f04610491ce7/src/hotspot/share/compiler/compileBroker.cpp#L2347-L2352

-------------

PR: https://git.openjdk.java.net/jdk/pull/2543

[Christian Hagedorn-3]

> 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

Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:

  Fix order of non_data_bits

-------------

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2543/files
  - new: https://git.openjdk.java.net/jdk/pull/2543/files/5fd1b911..82df324a

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2543&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2543&range=00-01

  Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/2543.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/2543/head:pull/2543

PR: https://git.openjdk.java.net/jdk/pull/2543

[Tobias Hartmann-3]

On Tue, 16 Feb 2021 11:07:14 GMT, Christian Hagedorn <[hidden email]> wrote:

>> 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
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix order of non_data_bits

Looks good to me.

src/hotspot/share/c1/c1_LIR.hpp line 235:

> 233:     , is_fpu_stack_offset_bits = 1        // used in assertion checking on x86 for FPU stack slot allocation
> 234:     , non_data_bits  = kind_bits + type_bits + size_bits + destroys_bits + last_use_bits +
> 235:                        is_fpu_stack_offset_bits + virtual_bits + is_xmm_bits + pointer_bits

Would be nice to have this in the same order as the enum values.

-------------

Marked as reviewed by thartmann (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2543

[Christian Hagedorn-3]

On Tue, 16 Feb 2021 10:45:38 GMT, Tobias Hartmann <[hidden email]> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix order of non_data_bits
>
> Looks good to me.

Thank you Tobias for your review!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2543

[Vladimir Kozlov-2]

On Tue, 16 Feb 2021 11:36:14 GMT, Christian Hagedorn <[hidden email]> wrote:

>> 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
>
> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>
>   Fix order of non_data_bits

Good.

-------------

Marked as reviewed by kvn (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2543

[Christian Hagedorn-3]

On Tue, 16 Feb 2021 22:08:16 GMT, Vladimir Kozlov <[hidden email]> wrote:

>> Christian Hagedorn has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix order of non_data_bits
>
> Good.

Thanks Vladimir for your review!

-------------

PR: https://git.openjdk.java.net/jdk/pull/2543

[Christian Hagedorn-3]

On Fri, 12 Feb 2021 10:03:25 GMT, Christian Hagedorn <[hidden email]> wrote:

> 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

This pull request has now been integrated.

Changeset: 84182855
Author:    Christian Hagedorn <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/84182855
Stats:     4152 lines in 6 files changed: 4137 ins; 2 del; 13 mod

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

Reviewed-by: thartmann, kvn

-------------

PR: https://git.openjdk.java.net/jdk/pull/2543