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

[Vladimir Kozlov]

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


The last change in compile.cpp seems unrelated:

+     if (AlwaysIncrementalInline) {
+       inline_incrementally(igvn);
+     }

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