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

[Vladimir Ivanov]

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

So, I don't see any problem with that code.

Best regards,
Vladimir Ivanov