[15] RFR(S): 8244724: CTW: C2 compilation fails with "Live Node limit exceeded limit"

[Vladimir Kozlov]

On 6/29/20 1:49 AM, Christian Hagedorn wrote:
> Thank you Nils and Vladimir for your reviews!
> 
> On 27.06.20 00:35, Vladimir Kozlov wrote:
>> You don't need to use 'C->' in Compile::Optimize() method:
>>
>> DEBUG_ONLY(C->set_phase_optimize_finished();)
> 
> Oh, right!
> 
>> Yes, remove all nodes limit checks after optimization phase.
> 
> I created a new webrev with all those Compile::check_node_count() calls removed and the change above:
> http://cr.openjdk.java.net/~chagedorn/8244724/webrev.02/
> 
> However, since these calls also affect bailout decisions in the product version and this bug is targeted for 15, I 
> suggest to remove these calls/bailouts for 16 only in a separate RFE to minimize the risk.

Agree. And please keep third failure check in chaitin.cpp because it also follows Split():

  538     if (C->failing()) {
  539       return;
  540     }

It seems okay to remove 1st and 4th checks as you did.

Thanks,
Vladimir

> 
> Best regards,
> Christian
> 
>> Thanks,
>> Vladimir
>>
>> On 6/26/20 7:38 AM, Christian Hagedorn wrote:
>>> Hi
>>>
>>> Please review the following patch:
>>> https://bugs.openjdk.java.net/browse/JDK-8244724
>>> http://cr.openjdk.java.net/~chagedorn/8244724/webrev.01/
>>>
>>> The testcase contains many string concatinations. These are compiled by javac with -XDstringConcat=inline which 
>>> creates a lot of StringBuilder objects and calls. As a result, we get a huge graph and eventually hit the live node 
>>> limit assert during code generation when trying to create new nodes - either during PhaseCFG::build_cfg() or later in 
>>> PhaseCFG::global_code_motion().
>>>
>>> We could try to introduce estimates for them to bailout but that appears to be difficult to get right without being 
>>> too pessimistic about it. But we need to be in order to avoid hitting the assert again by just modifying the testcase.
>>>
>>> Therefore, my suggestion is to completely skip the assert once the optimization phase is finished as we should not 
>>> strictly care about the node limit anymore at this point in time and it does not really provide much help for finding 
>>> bugs.
>>>
>>> A question remains, though, if we should also get rid of the remaining live node limit bailout checks in 
>>> Compile::Code_Gen() like [1] as it appears to be a waste to go through all the optimization in the optimization phase 
>>> to then bailout while generating code based only on the live node limit itself. What do you think about that?
>>>
>>> I also updated "<" into "<=" in the live node limit assert because we should be allowed to reach the limit but not go 
>>> beyond.
>>>
>>> Thank you!
>>>
>>> Best regards,
>>> Christian
>>>
>>>
>>> [1] http://hg.openjdk.java.net/jdk/jdk/file/8fd3e34e8379/src/hotspot/share/opto/coalesce.cpp#l236