RFR: 8201543: Modularize C1 GC barriers

[Erik Österlund]

Hi Per,

Thank you for the review. I have incorporated your suggested improvements.

/Erik

On 2018-04-26 12:44, Per Liden wrote:
> On 04/26/2018 12:38 PM, Per Liden wrote:
> [...]
>>>
>>> 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.
>
>
> Btw, I don't need to see a new webrev.
>
> /Per