RFR: JDK-8213489: GC/C2 abstraction for Compile::final_graph_reshaping() (original) (raw)

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Nov 7 22:55:49 UTC 2018

On 11/7/18 1:40 PM, Roman Kennke wrote:

Hi Vladimir,

Next

+        assert(n->isMem(), ""); +        MemNode* mem  = (MemNode*)n; could be replaced with +        MemNode* mem  = n->asMem(); Right. I copied existing code from finalgraphreshaping. New changeset fixes it.

Would be nice if you also fix existing code which you copied.

I don't see removal of moved ZGC code in Compile::finalgraphreshapingimpl() Oops, my bad. Fixed. Why you skip only assert and not whole switch() (or return) ? Do you want to process some nodes twice: by GC first and then in main switch? Yes. We have a case in Shenandoah where we want to process a CallNode Shenandoah-specific and then also want to verify/reshape generically.

What "verify/reshape generically" code you are referencing here? If opcode of Shenandoah's node is not listed in main switch only 'default:' code will be executed. And you are skipping code there in such case. I still do not get why you need to execute main switch when gc_handled is 'true'.

Do you want to process any nodes already listed in main switch? In such cases you would need to return 'false' and execute main switch - asserts will be valid in such case.

May be pass nop (opcode) to finalgraphreshaping() too to avoid virtual call Opcode() there. > OK You did not replace Opcode() call in ZBarrierSetC2::final_graph_reshaping():

switch (n->Opcode()) {

Thanks, Vladimir

Incremental: http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.01.diff/ Full: http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.01/ Better? Thanks for reviewing! Roman

Thanks, Vladimir

On 11/7/18 9:00 AM, Roman Kennke wrote: GCs might want to do something to nodes in Compile::finalgraphreshaping(). Let's put an abstraction in this place.

The way I did it was to put a call into BSC2::finalgraphreshaping(Compile*, Node*) in front of the huge switch and let the caller know if the node was handled or not. This is subsequently checked in the default-branch: if GC handled it, the asserts are not checked. This should provide the exact same behaviour that we have now, only better and nicer. I also took the opportunity and moved the ZGC related parts there to ZBarrierSetC2. Bug: https://bugs.openjdk.java.net/browse/JDK-8213489 Webrev: http://cr.openjdk.java.net/~rkennke/JDK-8213489/webrev.00/ Testing: hotspot/jtreg:tier1 passes Thoughts? Reviews? Roman

More information about the hotspot-gc-dev mailing list