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

[Vladimir Kozlov]

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