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

[Emanuel Peter]

On Tue, 21 Mar 2023 07:59:46 GMT, Tobias Hartmann <thartmann at openjdk.org> wrote:

>> Emanuel Peter has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   addressing Vladimir K's review suggestions
>
> 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?

Ok, I will remove he question. I think you are right, we should at least still have C1 compilation. In my experiments, it was extremely rare, this case.

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

It sounds likely, but I never saw it happen, up to tier9. So I suggest we leave the assert in, and once it is triggered, we can decide to remove it again. But then we also have an example, and we can check it in as a test, to justify the bailout.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1144787843
PR Review Comment: https://git.openjdk.org/jdk/pull/13038#discussion_r1144786768