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

Best regards,
Vladimir Ivanov

[1] 
http://hg.openjdk.java.net/jdk/jdk/file/tip/src/hotspot/share/opto/memnode.cpp#l1442

>>
>> Also, it's possible to do everything in a single pass, but it requires 
>> eager allocation of IDs for not-yet-seen nodes (which needs 
>> _new2old_map).
>>
>> Best regards,
>> Vladimir Ivanov
>>
>>> On 1/31/19 11:10 AM, Vladimir Ivanov wrote:
>>>> https://bugs.openjdk.java.net/browse/JDK-8217918
>>>> http://cr.openjdk.java.net/~vlivanov/8217918/webrev.00/
>>>>
>>>> When -XX:+AggressiveUnboxing is enabled, 
>>>> LoadNode::split_through_phi() produces Phi nodes with non-negative 
>>>> _inst_mem_id & _inst_id early enough, so it breaks PhaseRenumber 
>>>> pass which doesn't support nodes with embedded IDs.
>>>>
>>>> Proposed fix tracks nodes with embedded IDs and updates them once 
>>>> renumbering pass over the graph is over.
>>>>
>>>> Testing: hs-precheckin-comp, tier1-5
>>>>
>>>> Best regards,
>>>> Vladimir Ivanov