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

[Liu, Xin]

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

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. 

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();
   }


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