[9] RFR(M): 8040213: C2 does not put all modified nodes on IGVN worklist

[Vladimir Kozlov]

Initial prototype was written by me so part of changes are mine.

On 7/17/14 3:40 AM, Tobias Hartmann wrote:
> Hi John,
>
> thanks a lot for the detailed review! Please see comments inline.
>
> On 16.07.2014 01:10, John Rose wrote:
>>>> Done. New webrev:
>>>> http://cr.openjdk.java.net/~thartmann/8040213/webrev.01/
>> Loading C->_modified_nodes with a stack address is bad news, even if
>> the code apparently cleans it up on exit.  (I say "apparently" because
>> there are early exits from that block, and the cleanup is not RAII
>> style.)  I strongly suggest heap allocation instead of stack
>> allocation; it's expensive but it's for debug code.

The list array allocation is done in compiler arena which is cleaned 
after compilation finished by ResourceMark, only descriptor is on stack:

+   Unique_Node_List modified_nodes(comp_arena());

It is our normal style. We do it a lot, see Compile::Code_Gen().

Tobias, if you want to allocate Unique_Node_List object on arena:

_modified_nodes = new (comp_arena()) Unique_Node_List(comp_arena());

>
> There are other places in the code where this is done, for example in
> Compile::Compile(..) the Unique_Node_List 'for_igvn' is created on the
> stack. Is this a special case?
>
> I'm not sure how to correctly allocate the object on the heap. Do I have
> to use new/delete or is this handled by some magic in "ResourceObject'?
>
>
>> You inserted '#ifdef ASSERT' after
>> 'PhaseIterGVN::verify_PhaseIterGVN', but I don't think you need it,
>> since all of the code (including the similar insertion a few lines up)
>> is '#ifndef PRODUCT'.  See also suggestion above regarding
>> 'modified_nodes'.

#ifndef PRODUCT is also 'optimized' version of VM. n->dump() method is 
not defined in optimized build. That is why asserts are needed.

Tobias, please, build 'optimized' VM to verify your changes.

Thanks,
Vladimir

>
> True, I removed the #ifdef.
>
>> There are a few places where tests like 'while
>> (modified_list->size())' appears but there is no null check of
>> 'modified_list' itself.  This could cause a crash if we ever get the
>> phasing wrong; consider an assert or explicit null check 'while
>> (modified_list != NULL && ...)'.
>
> I added explicit null checks.
>
>
> New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.02/
>
> Thanks,
> Tobias
>
>> — John