RFR(M): 8026796: Make replace_in_map() on parent maps generic

[Roland Westrelin]

Anyone else for this review?

Roland.


On Jun 3, 2014, at 8:31 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:

> Thanks for review, Vladimir.
> 
> Roland.
> 
> On Jun 3, 2014, at 8:29 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
>> On 6/3/14 5:05 AM, Roland Westrelin wrote:
>>> Hi Vladimir,
>>> 
>>>> May be you should move ReplacedNodes into new files, callnode.* files are big already.
>>>> 
>>>> When you path ReplacedNodes as parameter can you use & reference instead of copy constructor?:
>>>> 
>>>> +   void transfer_from(ReplacedNodes other, uint idx);
>>>> +   void merge_with(ReplacedNodes other);
>>> 
>>> I followed both suggestions. Here is a new webrev:
>>> 
>>> http://cr.openjdk.java.net/~roland/8026796/webrev.02/
>> 
>> Looks good.
>> 
>>> 
>>>> The last change in compile.cpp seems unrelated:
>>>> 
>>>> +     if (AlwaysIncrementalInline) {
>>>> +       inline_incrementally(igvn);
>>>> +     }
>>> 
>>> 
>>> It is unrelated but I used AlwaysIncrementalInline to stress test the code that operates on replaced nodes after late inlining. And this is small fix to AlwaysIncrementalInline that I’d like to push.
>> 
>> Okay.
>> 
>> Thanks,
>> Vladimir
>> 
>>> 
>>> Roland.
>>> 
>>>> 
>>>> Thanks,
>>>> Vladimir
>>>> 
>>>> On 5/23/14 7:48 AM, Roland Westrelin wrote:
>>>>> Hi Vladimir,
>>>>> 
>>>>> Here is a new webrev that implements your suggestions:
>>>>> 
>>>>> http://cr.openjdk.java.net/~roland/8026796/webrev.01/
>>>>> 
>>>>> Some comments below:
>>>>> 
>>>>>> Why previous code 8024069 does not work?
>>>>>> 
>>>>>> 'before', 'after' may not good names. You call new nodes 'improved' in the comment you should use it.
>>>>>> 
>>>>>> You meaningful names (not o,n) in record(Node* o, Node* n).
>>>>>> 
>>>>>> It would be nice to use getter/setter functions:
>>>>>> 
>>>>>> ReplacedNodes r = kit.map()->_replaced_nodes;
>>>>>> kit.map()->_replaced_nodes = r;
>>>>>> 
>>>>>> Do you have cases when ReplacedNodes could be repopulated? So that we reset length GrowableArray::clear() instead of trashing pointer:
>>>>>> 
>>>>>> +void ReplacedNodes::reset() {
>>>>>> +  _replaced_nodes = NULL;
>>>>>> +}
>>>>> 
>>>>> I don’t think so but it can’t hurt so I’ve made that change.
>>>>> 
>>>>>> I don't see cleanup of ReplacedNodes in Node::destruct() and other places where we clean up expensive nodes, for example.
>>>>> 
>>>>> I added the cleanup to Node::destruct(). Compile::remove_useless_nodes() does the cleanup as well. I don’t understand why it would be needed elsewhere.
>>>>> 
>>>>>> 
>>>>>> Is the code in Parse::create_entry_map() correct? First, you destroy ReplacedNodes:
>>>>>> 
>>>>>> _caller->map()->delete_replaced_nodes();
>>>>>> 
>>>>>> then you used it to initialize new map:
>>>>>> 
>>>>>> SafePointNode* inmap = _caller->map();
>>>>>> map()->transfer_replaced_nodes_from(inmap, _new_idx);
>>>>> 
>>>>>    GraphKit kit(_caller);
>>>>>    kit.null_check_receiver_before_call(method());
>>>>> 
>>>>> between the call to _caller->map()->delete_replaced_nodes() and the map()->transfer_replaced_nodes_from(inmap, _new_idx).
>>>>> 
>>>>> calls replace_in_map() with the caller’s map. map()->transfer_replaced_nodes_from(inmap, _new_idx) is so that we don’t lose track of it.
>>>>> 
>>>>> Roland.
>>>>> 
>>>>>> 
>>>>>> 
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>> 
>>>>>> On 5/19/14 4:37 AM, Roland Westrelin wrote:
>>>>>>> I forgot the webrev:
>>>>>>> 
>>>>>>> http://cr.openjdk.java.net/~roland/8026796/webrev.00/
>>>>>>> 
>>>>>>> Roland.
>>>>>>> 
>>>>>>> On May 19, 2014, at 1:01 PM, Roland Westrelin <roland.westrelin at oracle.com> wrote:
>>>>>>> 
>>>>>>>> This change reverts:
>>>>>>>> 
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8024069
>>>>>>>> 
>>>>>>>> and propagates replacements in replace_in_map() to callers in a generic way. Every time replace_in_map() is called, the pair of nodes passed to replace_in_map is pushed on a list that the current map carries. When control flow paths merge, the lists for each of the control flow path’s maps are also merged. When parsing exits a method to return to a caller, the replaced nodes on the exit path are used to update the caller's map. This change also propagates replaced nodes after late inlining.
>>>>>>>> 
>>>>>>>> Roland.
>>>>>>> 
>>>>> 
>>> 
>