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

[Vladimir Kozlov]

On 2/1/19 10:33 AM, 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.

Vladimir K

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