Request for reviews (S): 6860599: Relax nodes limit check for Output phase

[Tom Rodriguez]

On Jul 20, 2009, at 2:57 PM, Vladimir Kozlov wrote:

> There is no compilation bailout in Output() and I don't
> want to add one. I think, to have the assert is enough.
>
> And I don't think bailout limits are the problem.
> We should finish code generation if the compilation passed
> bailouts during optimizations and RA.

I interpret that as meaning we're missing appropriate bailouts before  
we start to ensure that we can complete code generation.

>
> I agree that instead of simple doubling I can use something like this:
>
> _nodes_limit = _nodes_limit + NodeLimitFudgeFactor +
>               ncalls*3 + nloops*(OptoLoopAlignment-1)))

Can't you just a put in a check_node_count call before we start Output  
to check that we have this margin left?

tom

>
> Thanks,
> Vladimir
>
> Tom Rodriguez wrote:
>> On Jul 20, 2009, at 12:53 PM, Vladimir Kozlov wrote:
>>> Tom,
>>>
>>> I do not want disable the assert since it may catch a situation
>>> when something goes wrong in Output(), I want relax it.
>> Oh.  Doubling it seems like disabling it to me.  Relaxing it would  
>> be increasing it slightly or increasing the limit based on the  
>> amount we actually expect it to increase by.  Isn't the real  
>> problem that the bailout limits aren't triggering when they  
>> should?  I'd rather see proper bailout bounds instead of just  
>> moving the MaxNodeLimit around.
>> tom
>>>
>>> I can use an other flag instread of doubling:
>>>
>>>  void increase_nodes_limit() { _nodes_limit = CodeGenMaxNodeLimit; }
>>>
>>> And I can use nodes_limit() only in Node::verify_construction()
>>> if everybody don't want changes in other places where MaxNodeLimit
>>> is used.
>>> Thanks,
>>> Vladimir
>>>
>>> Tom Rodriguez wrote:
>>>> If the intent is just to suppress the assert after a certain  
>>>> point in code generation, why don't you just add a flag in the  
>>>> Compile for that?  Adding _node_limit and just doubling it seems  
>>>> arbitrary and doesn't really capture intent.
>>>> tom
>>>> On Jul 20, 2009, at 9:32 AM, Vladimir Kozlov wrote:
>>>>> Christian,
>>>>>
>>>>> The only nodes limit check done in Output is in
>>>>> Node::verify_construction() and it is assert()
>>>>> which only works in debug mode.
>>>>> Did I understand your question correctly?
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>
>>>>> Christian Thalinger wrote:
>>>>>> Vladimir Kozlov wrote:
>>>>>>> http://cr.openjdk.java.net/~kvn/6860599/webrev.00
>>>>>>>
>>>>>>> Fixed 6860599: Relax nodes limit check for Output phase
>>>>>>>
>>>>>>> Problem:
>>>>>>> I got several CTW cases when without EA C2 "gracefully"
>>>>>>> bailout compilation when nodes limit check failed during
>>>>>>> macro nodes expansion. And with EA it passed macro nodes
>>>>>>> expansion but crashed with ASSERT during Output phase.
>>>>>>> One byte MachNop nodes are used in debug mode for loops
>>>>>>> and calls alignment in Output phase. As result for a big
>>>>>>> method the node limit could be reached.
>>>>>>>
>>>>>>> Solution:
>>>>>>> Increase nodes limit (double) for Output phase.
>>>>>> What I don't understand with this patch is, it changes the node  
>>>>>> limit
>>>>>> but this is done for every output.  It's not limited to e.g.  
>>>>>> debug mode.
>>>>>> Is this what you indented?
>>>>>> -- Christian