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

[Tobias Hartmann]

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