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

[Tobias Hartmann]

Thank you, Vladimir.

Best,
Tobias

On 23.07.2014 16:35, Vladimir Kozlov wrote:
> Good.
>
> Thanks,
> Vladimir
>
> On 7/23/14 5:46 AM, Tobias Hartmann wrote:
>> Hi Vladimir,
>>
>> thanks for the review.
>>
>> On 22.07.2014 19:20, Vladimir Kozlov wrote:
>>> C->modified_nodes() is called only from debug code so you can 
>>> simplify it by putting whole method inside DEBUG_ONLY()
>>> instead of different return:
>>>
>>> DEBUG_ONLY( Unique_Node_List* modified_nodes() const { return 
>>> _modified_nodes; } )
>>
>> Done: http://cr.openjdk.java.net/~thartmann/8040213/webrev.05/
>>
>> Thanks,
>> Tobias
>>
>>>
>>> Otherwise look good.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 7/22/14 1:15 AM, Tobias Hartmann wrote:
>>>> Hi Vladimir,
>>>>
>>>> On 21.07.2014 19:13, Vladimir Kozlov wrote:
>>>>> > Thanks for the hint, the optimized build did not work. I'm now 
>>>>> using
>>>>> > PRODUCT_RETURN and NOT_PRODUCT to guard the verification code. 
>>>>> This is
>>>>> > consistent with other verification methods like
>>>>> > Compile::verify_graph_edges(..).
>>>>>
>>>>> The logic which verifies modified nodes in phaseX.cpp is under #ifdef
>>>>> ASSERT. Yes, you are not collecting data in optimized VM by returning
>>>>> NULL from modified_nodes(). But it is very confusing. I would suggest
>>>>> to move all this code to debug build only. You can use
>>>>> NOT_DEBUG_RETURN instead of PRODUCT_RETURN and put definitions under
>>>>> #ifdef ASSERT in compile.cpp file.
>>>>
>>>> Okay, that's true. I missed that there is a NOT_DEBUG_RETURN. Now 
>>>> using it.
>>>>
>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.04/
>>>>
>>>> Thanks,
>>>> Tobias
>>>>
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 7/21/14 2:50 AM, Tobias Hartmann wrote:
>>>>>> Vladimir, John, thanks for the review.
>>>>>>
>>>>>> On 18.07.2014 01:32, John Rose wrote:
>>>>>>> On Jul 17, 2014, at 3:32 PM, Vladimir Kozlov
>>>>>>> <vladimir.kozlov at oracle.com> wrote:
>>>>>>>
>>>>>>>> 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().
>>>>>>> Yes, we do it when adding phase control blocks to the Compile 
>>>>>>> object,
>>>>>>> within the tree of calls rooted at the constructor.  I see that the
>>>>>>> new code is arguably part of that pattern.  I hope that is an
>>>>>>> exceptional use of this style?
>>>>>>>
>>>>>>> Note that C2Compiler::compile_method creates the Compile object and
>>>>>>> then queries it.  When it queries it, the Compile object may 
>>>>>>> contain
>>>>>>> dangling pointers into phase control blocks stored on the stack
>>>>>>> (during subroutine calls like Code_Gen) but now deallocated.
>>>>>>>
>>>>>>> It seems to me that we avoid touching those dangling pointers by
>>>>>>> simple will power and discipline.   Such a style doesn't scale, 
>>>>>>> since
>>>>>>> it's too easy to make mistakes as the code gets complicated.
>>>>>>>
>>>>>>>> Tobias, if you want to allocate Unique_Node_List object on arena:
>>>>>>>>
>>>>>>>> _modified_nodes = new (comp_arena()) 
>>>>>>>> Unique_Node_List(comp_arena());
>>>>>>> That makes me feel much better.
>>>>>>>
>>>>>>> I would prefer for us to do it this way for the other stack-chunks
>>>>>>> also, but that would be outside the scope of this bug.
>>>>>>
>>>>>> I changed it to heap allocation.
>>>>>>
>>>>>>> Tobias, thanks for doing this work.
>>>>>>>
>>>>>>> — John
>>>>>>>
>>>>>>>>> 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 for the hint, the optimized build did not work. I'm now using
>>>>>> PRODUCT_RETURN and NOT_PRODUCT to guard the verification code. 
>>>>>> This is
>>>>>> consistent with other verification methods like
>>>>>> Compile::verify_graph_edges(..).
>>>>>>
>>>>>> New webrev: http://cr.openjdk.java.net/~thartmann/8040213/webrev.03/
>>>>>>
>>>>>> Thanks,
>>>>>> Tobias
>>>>>>
>>>>>>>> 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
>>>>
>>