RFR: 8303951: Add asserts before record_method_not_compilable where possible [v2]

[Tobias Hartmann]

On Mon, 20 Mar 2023 13:58:52 GMT, Emanuel Peter <epeter at openjdk.org> wrote:

>> I went through all `C2` bailouts, and checked if they are justified to bail out of compilation silently. I added asserts everywhere. Those that were hit, I inspected by hand.
>> 
>> Some of them seem to be justified. There I added comments why they are justified. They are cases that we do not want to handle in `C2`, and that are rare enough so that it probably does not matter.
>> 
>> For the following bailouts I did not add an assert, because it may have revealed a bug:
>> [JDK-8304328](https://bugs.openjdk.org/browse/JDK-8304328) C2 Bailout "failed spill-split-recycle sanity check" reveals hidden issue with RA
>> 
>> Note:
>> [JDK-8303466](https://bugs.openjdk.org/browse/JDK-8303466) C2: COMPILE SKIPPED: malformed control flow - only one IfProj
>> That bug bug was the reason for this RFE here. I added the assert for "malformed control flow". After this RFE here, that Bug will run into the assert on debug builds.
>> 
>> I ran `tier1-6` and stress testing. Now running `tier7-9`.
>> 
>> Filed a follow-up RFE to do the same for `BAILOUT` in `C1`: [JDK-8304532](https://bugs.openjdk.org/browse/JDK-8304532).
>
> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
> 
>   addressing Vladimir K's review suggestions

Looks good overall, some comments below.

src/hotspot/share/opto/matcher.cpp line 1256:

> 1254:       out_arg_limit_per_call = OptoReg::add(warped,1);
> 1255:     if (!RegMask::can_represent_arg(warped)) {
> 1256:       // Bailout. For example not enoug space on stack for all arguments. Happens for methods with too many arguments.

Suggestion:

      // Bailout. For example not enough space on stack for all arguments. Happens for methods with too many arguments.

src/hotspot/share/opto/parse1.cpp line 211:

> 209:   // of loops in catch blocks or loops which branch with a non-empty stack.
> 210:   if (sp() != 0) {
> 211:     // Bailout. But we should probably kick into normal compilation?

We shouldn't add a question which is equivalent to a ToDo (same below). The comment should explain how this could happen and if we think that making the method not compilable is too strong, we should file a follow-up issue to investigate/fix.

How common is this? We will still compile at C1, so normal compilation **will** kick in, right?

src/hotspot/share/opto/parse1.cpp line 218:

> 216:   if (osr_block->has_trap_at(osr_block->start())) {
> 217:     assert(false, "OSR starts with an immediate trap");
> 218:     // Bailout. But we should probably kick into normal compilation?

"OSR inside finally clauses" sounds like it could easily happen.

src/hotspot/share/opto/parse1.cpp line 436:

> 434:   _flow = method()->get_flow_analysis();
> 435:   if (_flow->failing()) {
> 436:     assert(false, "flow fails during parsing");

Suggestion:

    assert(false, "type flow failed during parsing");

src/hotspot/share/opto/parse1.cpp line 515:

> 513:     _flow = method()->get_osr_flow_analysis(osr_bci());
> 514:     if (_flow->failing()) {
> 515:       assert(false, "OSR flow failure");

Suggestion:

      assert(false, "type flow analysis failed for OSR compilation");

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

PR Review: https://git.openjdk.org/jdk/pull/13038#pullrequestreview-1349839442
PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1142981232
PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1142999117
PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1142994434
PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1142982576
PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1142983628