RFR(S): 8219448: split-if update_uses accesses stale idom data

[Nils Eliasson]

Updated webrev:

http://cr.openjdk.java.net/~neliasso/8219448/webrev.02

Regards,

Nils

On 2019-02-28 22:55, Vladimir Kozlov wrote:
> Got it. In such case you need to replace lazy_replace() at line 575 
> with remove_dead_node() and you don't need assert outcnt() == 0 then.
>
> Thanks,
> Vladimir
>
> On 2/28/19 11:48 AM, Nils Eliasson wrote:
>> Hi,
>>
>> I just moved it up from line 570.
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/196ab0abc685/src/hotspot/share/opto/split_if.cpp#l570 
>>
>>
>> And on 575 we will call the replace on it, to finally kill it.
>>
>> http://hg.openjdk.java.net/jdk/jdk/file/196ab0abc685/src/hotspot/share/opto/split_if.cpp#l575 
>>
>>
>> So the node is dead, we just must handle the phi-uses first, and 
>> while doing that, a correct idom is required.
>>
>> The code that triggers this bug has a diamond-shaped control flow 
>> below the split-if-region. The region-nodes that need their idom 
>> corrected is far down and isn't touched during the split. But there 
>> is a call down there. It has one of its data edges defined by a phi 
>> hanging on the split-region. So when we try to call spinup on it, it 
>> will traverse a broken idom chain.
>>
>> The conclusion of my investigation is that all regions that have the 
>> split-region as its idom, must be updated (even if they are below). 
>> For that we have the lazy_update mechanism, and to make it trigger, I 
>> must mark the region as killed slightly earlier.
>>
>> Regards,
>>
>> Nils
>>
>>
>>
>>
>>
>> On 2019-02-28 20:00, Vladimir Kozlov wrote:
>>> Hi Nils,
>>>
>>> You are updating map so that next code in idom_no_update() works for 
>>> you:
>>> http://hg.openjdk.java.net/jdk/jdk/file/196ab0abc685/src/hotspot/share/opto/loopnode.hpp#l928 
>>>
>>> Which seems a hack to me. I think we should fix spinup() method to 
>>> skip old region when looking for idom (I assume that is where you 
>>> have the problem).
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 2/28/19 5:18 AM, Nils Eliasson wrote:
>>>> Hi,
>>>>
>>>> This patch fixes some of the idom updates in split-if. The updates 
>>>> are there, but at the end, which is to late. They need to be 
>>>> correct when handling the uses of the region being split. When the 
>>>> stale idom is seen we end up asserting or crashing.
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8219448
>>>>
>>>> http://cr.openjdk.java.net/~neliasso/8219448/webrev.01/
>>>>
>>>> Please review,
>>>>
>>>> Nils Eliasson
>>>>