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

[Roland Westrelin]

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/

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

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