RFR: 8201543: Modularize C1 GC barriers

[Per Liden]

Hi Erik,

On 04/13/2018 05:11 PM, Erik Österlund wrote:
> Hi,
> 
> The GC barriers for C1 are not as modular as they could be. It currently 
> uses switch statements to check which GC barrier set is being used, and 
> calls one or another barrier based on that, in a way that it can only be 
> used for write barriers.
> 
> The solution I propose is to add the same facilities that have been 
> added in runtime and the interpreter already: a barrier set backend for 
> C1. I call it BarrierSetC1, and it helps us generate decorated accesses 
> that give the GC control over the details how to generate this access. 
> It recognizes the same decorators (accessDecorators.hpp) that the other 
> parts of the VM recognize. Each concrete barrier set has its own 
> backend. For now, these are CardTableBarrierSetC1 and G1BarrierSetC1, 
> but this should pave way for upcoming concurrently compacting GCs as well.
> 
> Two decorators were added for C1 specifically (found in c1_Decorators.hpp):
> C1_NEEDS_PATCHING for accesses where the index is not yet load because 
> the class has yet to be loaded, and
> C1_MASK_BOOLEAN for accesses that need to mask untrusted boolean values.
> 
> LIRGenerator calls a wrapper called access_store_at, access_load_at, etc 
> (there are variants for cpmxchg, xchg and atomic add as well).
> The access call calls straight into the BarrierSetC1 hierarchy using 
> virtual calls. It is structured in a way very similar to 
> BarrierSetAssembler.
> 
> BarrierSetC1 can also be called during initialization to generate stubs 
> and runtime methods required by C1. For G1BarrierSetC1, this results in 
> calling the BarrierSetAssembler for the platform specific code. This 
> way, the BarrierSetC1 hierarchy has been carefully kept in shared code, 
> and the switch statements for generating G1 code have been removed. Some 
> code that used to be platform specific (like unsafe get/set and array 
> store) have been broken out to shared code, with the actual platform 
> specific details (some register allocation for store check and atomics) 
> broken out to platform specific methods. This way, calls to access are 
> kept in platform specific code.
> 
> As usual, big thanks go to Martin Doerr for helping out with S390 and 
> PPC, and Roman for taking care of AArch64.
> 
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8201543
> 
> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8201543/webrev.00/


Looks good overall, just some minor comments.


src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.hpp
-------------------------------------------------------
59   void gen_g1_pre_barrier_stub(LIR_Assembler* ce, G1PreBarrierStub* 
stub);
60   void gen_g1_post_barrier_stub(LIR_Assembler* ce, G1PostBarrierStub* 
stub);

It seems superfluous to have "g1" in the function names given the context ;)


src/hotspot/cpu/x86/gc/g1/g1BarrierSetAssembler_x86.cpp
-------------------------------------------------------
The C1-related includes and functions should be under #ifdef COMPILER1.


src/hotspot/share/gc/shared/c1/barrierSetC1.hpp
-----------------------------------------------

  96   CodeEmitInfo*& patch_info()          { return _patch_info; }
  97   CodeEmitInfo*& access_emit_info()    { return _access_emit_info; }

Should we make this "patch_info" and "access_info"? Or maybe 
"patch_emit_info" and "access_emit_info"? I think I prefer the former.


122   virtual LIR_Opr atomic_cmpxchg_resolved(LIRAccess& access, 
LIRItem& cmp_value, LIRItem& new_value);
123
124   virtual LIR_Opr atomic_xchg_resolved(LIRAccess& access, LIRItem& 
value);
...
133   virtual LIR_Opr atomic_xchg(LIRAccess& access, LIRItem& value);

The methods above are missing an "_at" in their name.


src/hotspot/share/gc/shared/c1/barrierSetC1.cpp
-----------------------------------------------

  73 void BarrierSetC1::store_at(LIRAccess& access,LIR_Opr value) {

Missing space in argument list.


src/hotspot/share/runtime/sharedRuntime.cpp
-------------------------------------------
The only change in this file is a single (unnecessary) line delete.


cheers,
Per

> 
> Thanks,
> /Erik