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

[Vladimir Kozlov]

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