[13] RFR (S): 8217918: C2: -XX:+AggressiveUnboxing is broken

[Vladimir Kozlov]

On 2/1/19 3:15 PM, Vladimir Ivanov wrote:
> 
>>>>>> Nice! Did you consider to update indexes in separate pass at the end of PhaseRenumberLive? Would it be simpler?
>>>>>
>>>>> It already does additional pass, but only over the nodes which contain embedded IDs (those are recorded into 
>>>>> _delayed during the first pass). If you are talking about full pass over the graph, then it would allow to 
>>>>> eliminate _delayed array.
>>>>
>>>> No, I was only concern about code in new_index() where you store _live_node_count if Id is not set in _old2new_map.
>>>> I was thinking to collect all nodes with ID (regardless values in _old2new_map) first as you do now for _delayed. 
>>>> And then do the same as your code (call update_embedded_ids() in post loop) where you can assume that _old2new_map 
>>>> contains all values - so you can replace your checks with asserts. Am I missing something?
>>>
>>> I spotted the following code to produce Phi with instance_id which may become stale (base phi goes dead) by the time 
>>> RenumberLive kicks in:
>>>
>>> Node *LoadNode::split_through_phi(PhaseGVN *phase) {
>>> ...
>>>    if (!t_oop->is_known_instance() && load_boxed_values) {
>>>      // Use _idx of address base for boxed values.
>>>      this_iid = base->_idx;
>>>    }
>>>    PhaseIterGVN* igvn = phase->is_IterGVN();
>>>    Node* phi = new PhiNode(region, this_type, NULL, mem->_idx, this_iid, this_index, this_offset);
>>>
>>>
>>> It looks benign if we can guarantee that the ID won't be reused, so I allocate a placeholder ID.
>>
>> This is done for next:
>> http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/memnode.cpp#l1142
>>
>> This ID value can't be random one.
> 
> The code you refer to compares PHI nodes using is_same_inst_field() which looks for the node with the same inst_id 
> (among other things).
> 
> Placeholder ID is chosen to be unique (no node in the graph with the same ID is present or will be in the future), but 
> the ID is shared between all the embedded usages. It doesn't refer to any existing node, but it didn't before the 
> renumbering pass as well.

But before renumbering it could be the same ID - and now it will be different. What I am trying to say some nodes could 
have the same ID which came from already dead node. I think we should preserve that - keep the same ID (which could be 
different after renumbering.

Vladimir K

> 
> So, I don't see any problem with that code.
> 
> Best regards,
> Vladimir Ivanov