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

[Vladimir Kozlov]

+1

Thanks,
Vladimir

On 3/7/19 5:29 AM, Tobias Hartmann wrote:
> Hi Nils,
> 
> looks good to me.
> 
> Best regards,
> Tobias
> 
> On 07.03.19 14:17, Nils Eliasson wrote:
>> 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
>>>>>>