RFR(S) 8251271- C2: Compile::_for_igvn list is corrupted after RenumberLiveNodes

[Christian Hagedorn]

Hi Xin

On 03.09.20 19:29, Liu, Xin wrote:
> hi, Christian and Nhat,
> 
> I notice that for_igvn() is cleared before PhaseRenumberLive too,  but it's not true that it's an empty worklist in PhaseRenumberLive.
> 
> The base constructor PhaseRemoveUseless appends some "unique_out" to the for_igvn()
> Check out "record_for_igvn(n->unique_out())" in  C->remove_useless_nodes(_useful);

You're right! I missed that one.

> I guess the original code works because new_worklist is in the C->comp_arena.
> There's no demolition code for Unique_Node_List, so the object is still in good shape even it is out of scope.
> 
> No doubt that Nhat's patch restores the valid pointer. The bad thing is that its content is corrupted to use.
> We are lucky because nobody uses it to construct a PhaseIterGVN after RenumberLiveNodes.

Right.
> How about this? We can clear() the worklist out.
> 
> diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp
> --- a/src/hotspot/share/opto/compile.cpp
> +++ b/src/hotspot/share/opto/compile.cpp
> @@ -2089,9 +2089,12 @@
>         ResourceMark rm;
>         PhaseRenumberLive prl = PhaseRenumberLive(initial_gvn(), for_igvn(), &new_worklist);
>       }
> +    Unique_Node_List* save_for_igvn = for_igvn();
>       set_for_igvn(&new_worklist);
>       igvn = PhaseIterGVN(initial_gvn());
>       igvn.optimize();
> +    set_for_igvn(save_for_igvn);
> +    for_igvn()->clear();
>     }

This looks good while having no users afterwards. But I'm fine with both 
solutions.

Best regards,
Christian

> 
> thanks,
> --lx
> ________________________________________
> From: Christian Hagedorn <christian.hagedorn at oracle.com>
> Sent: Thursday, September 3, 2020 2:50 AM
> To: Liu, Xin; Nhat Nguyen; hotspot-compiler-dev at openjdk.java.net
> Subject: RE: [EXTERNAL] RFR(S) 8251271- C2: Compile::_for_igvn list is corrupted after RenumberLiveNodes
> 
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> 
> 
> 
> Hi Xin
> 
> I'm not sure if we really want to copy all the nodes from new_worklist
> back to for_igvn() when it's not used anymore afterwards. I thought that
> this RFE just intents to keep a valid pointer in Compile::_for_igvn.
> 
> Nevertheless, I also took a closer look at that code and it seems that
> for_igvn() is not even required for PhaseRenumberLive? It's cleared just
> before the phase on L2086 and then PhaseRenumberLive does not seem to
> add anything to it and neither does PhaseRemoveUseless. So we are
> basically restoring an empty list afterwards. However, this could be
> cleaned up in an additional RFE.
> 
> Best regards,
> Christian
> 
> On 03.09.20 10:56, Liu, Xin wrote:
>> hi, Nhat and Christian,
>>
>> I reviewed this patch. I feel resuming the old worklist looks suspicious.
>> PhaseRenumberLive renumbers nodes associates with gvn. old _worklist might have wrong idx  and types.
>>
>> Instead of resuming old _worklist,  you can copy out nodes from new_worklist to your current _worklist.
>>
>> diff --git a/src/hotspot/share/opto/compile.cpp b/src/hotspot/share/opto/compile.cpp
>> --- a/src/hotspot/share/opto/compile.cpp
>> +++ b/src/hotspot/share/opto/compile.cpp
>> @@ -2089,7 +2089,10 @@
>>          ResourceMark rm;
>>          PhaseRenumberLive prl = PhaseRenumberLive(initial_gvn(), for_igvn(), &new_worklist);
>>        }
>> -    set_for_igvn(&new_worklist);
>> +    for_igvn()->clear();
>> +    while (new_worklist.size() > 0) {
>> +     for_igvn()->push(new_worklist.rpop());
>> +    }
>>        igvn = PhaseIterGVN(initial_gvn());
>>        igvn.optimize();
>>      }
>>
>>
>> thanks,
>> --lx
>>
>> ________________________________________
>> From: hotspot-compiler-dev <hotspot-compiler-dev-retn at openjdk.java.net> on behalf of Christian Hagedorn <christian.hagedorn at oracle.com>
>> Sent: Thursday, August 27, 2020 7:54 AM
>> To: Nhat Nguyen; hotspot-compiler-dev at openjdk.java.net
>> Subject: RE: [EXTERNAL] RFR(S) 8251271- C2: Compile::_for_igvn list is corrupted after RenumberLiveNodes
>>
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>
>>
>>
>> Hi Nhat
>>
>> Looks good to me!
>>
>> Just make sure you that next time you assign the bug to you or a sponsor
>> and/or leave a comment that you intend to work on it to avoid the
>> possibility of some duplicated work (was no problem in this case) ;-)
>>
>> Best regards,
>> Christian
>>
>> On 26.08.20 20:55, Nhat Nguyen wrote:
>>> Hi hotspot-compiler-dev,
>>>
>>> Please review the following patch to address https://bugs.openjdk.java.net/browse/JDK-8251271
>>> The bug is currently assigned to Christian Hagedorn, but he was supportive of me submitting the patch instead.
>>> I have run hotspot/tier1 and jdk/tier1 tests to make sure that the change is working as intended.
>>>
>>> webrev: http://cr.openjdk.java.net/~burban/nhat/JDK-8251271/webrev.00/
>>>
>>> Thank you,
>>> Nhat
>>>