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

[Roland Westrelin]

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