Request for reviews (S): 6991188: C2 Crashes while compiling method

[Vladimir Kozlov]

Thank you, Ramki

How about this?:

   // Normally only 1-3 passes needed to build
   // Connection Graph depending on graph complexity.
   // Set limit to 10 to catch situation when something
   // did go wrong and recompile the method without EA.
#define CG_BUILD_ITER_LIMIT 10
   int iterations = 0;
   while(_progress && (iterations++ < CG_BUILD_ITER_LIMIT)) {

Thanks,
Vladimir

Y. S. Ramakrishna wrote:
> Hi Vladimir --
> 
> Just a general $0.02 comment, without my knowing anything about
> what is actually being done here.
> 
> Can you give an upper bound on the number of iterations
> that would be needed in terms of a suitable metric on the
> starting graph? A little more commentary on the upper bound
> (if a non-trivial one exists), and the choice and suitability of
> the default, would be nice.
> 
> Also, perhaps use a defined constant instead of the 10? (Would it
> be useful to have it be a diagnostic tunable?)
> 
> -- ramki
> 
> On 11/02/10 16:59, Tom Rodriguez wrote:
>> On Nov 2, 2010, at 4:42 PM, Vladimir Kozlov wrote:
>>
>>> Thanks, Tom
>>>
>>> Tom Rodriguez wrote:
>>>> Typo:
>>>> +     // Possible infinit build_connection_graph loop.
>>> Fixed.
>>>
>>>> Why 10?  How many passes does it normally take? 
>>> Normally only 1-2 passes depending on graph complexity. In the bug 
>>> test - 2, in JBB I saw 3 once.
>>>
>>>> Could this process only the nodes which changed or does it really 
>>>> need to reprocess every node?
>>> Only AddP, LoadP and StoreP (and narrow variants) are reprocessed. 
>>> All other nodes are marked in vectSet _processed. There is check at 
>>> the beginning of build_connection_graph().
>>> I thought about using a separate worklist in new iterating code and 
>>> populate it only with A/L/S nodes but I don't think it will help much.
>>> And we need to reprocess all A/L/S nodes since we don't know which 
>>> nodes will be affected by one change (the chain through deferred 
>>> edges could be long and split/merge through Phis).
>>
>> Sounds and looks good.
>>
>> tom
>>
>>> Thanks,
>>> Vladimir
>>>
>>>> tom
>>>> On Nov 2, 2010, at 3:27 PM, Vladimir Kozlov wrote:
>>>>> http://cr.openjdk.java.net/~kvn/6991188/webrev.00
>>>>>
>>>>> Fixed 6991188: C2 Crashes while compiling method
>>>>>
>>>>> Construction of Connection Graph during EA relied on
>>>>> DU relation corresponds to nodes index so that def
>>>>> nodes processed before their uses. Move EA after
>>>>> IGVN (for 6966411 fix broke that. Because of that
>>>>> not all Graph edges will be created leading to
>>>>> incorrect EA results and incorrect Ideal graph.
>>>>>
>>>>> Walk over interesting nodes (AddP, LoadP, StoreP)
>>>>> several times until no new edges are created.
>>>>> Set limit on iterations.
>>>>>
>>>>> Tested with assert in iterations bailout code and
>>>>> calls to PhaseIdealLoop::verify() (removed from
>>>>> final changes). Passed JPRT, full CTW, nsk.
>>>>>
>>