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

[Y. S. Ramakrishna]

On 11/03/10 10:16, Vladimir Kozlov wrote:
> 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)) {

I suppose it is not possible to say "The maximum number of
times we might need to iterate in the worst case is bounded
above by k*n, where k is the foo-metric of the graph and
n is its bar-metric", or ".. where foo is the number of
deferred edges and bar is the number of
nodes of type AddBaz in the starting graph", or some such
structural complexity metric/property of the code or the graph being
transformed?

What you have above looks good otherwise. (You have another use of
10 after the wile loop, at line 1610, which i am guessing you would also change
to use the defined constant.)

Also, what does the #iterations bound? The cost of the Escape Analysis,
or the size of the resulting code or both? (i.e. what kind of cost is
the iteration limit bounding?) It might be nice to mention that (unless
that is considered obvious to those familiar with the code).

Ah, may be there is no trivial upper bound, for you do say "possible infinite
build_connection_graph loop" at line 1611, which i missed on my
previous reading. That does make some of my comments above about
upper bound and cost-limiting somewhat moot.

Sorry again for my naive and nit-picky-sounding comments, stemming from my
lack of any bkgrd or knowledge of this code, so feel free to take all
of this with the right dose of salt (i.e. with lots of it) :-)

Thanks.
-- ramki

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