RFR: JDK-8207169: X86: Modularize cmpxchg-oop assembler for C1 and C2

[Erik Österlund]

Hi Roman,

I agree with you that it would be nice to have a good interface for 
this, and that putting your GC barriers straight into the AD file makes 
it very awkward to have a clean interface. It will be a special 
interface for shenandoah only, that only shenandoah uses. All other GCs 
generate nodes for their barriers. It seems like a bit of a shortcut for 
not having to generate the proper nodes for your barriers, which I think 
is the right level to do this, and is where all other GCs do this. The 
rules matched in the AD file kind of presume you know what instructions 
will be generated. This is required for example for PrintOptoAssembly to 
generate anything useful. It might not be a dealbreaker I suppose if 
PrintOptoAssembly prints nonsense. But I can't help but feel like the 
abstraction level is not right here, and that placing GC barriers 
straight into the AD file for convenience, is a bit of a hack.

An alternative that I think should be considered, is to do what ZGC 
does: generate nodes in ZBarrierSetC2, and macro expand barrier nodes in 
ZBarrierSetC2. That way, your changes can remain in your 
ShenandoahBarrierSetC2 class, and not leak out into the AD files.

Thanks,
/Erik

On 2018-08-29 16:35, Roman Kennke wrote:
> Hi Roland,
>
>>> It was suggested to me (off-list) to simply put this stuff under if
>>> (UseShenandoahGC). I don't have a very strong preference about this. I'm
>>> leaning towards having a (somewhat) proper GC interface for it, if only
>>> because of the symmetry with the runtime GC interface that also has a
>>> cmpxchg hook, and because, cmpxchg(-oop) is, in-fact, a heap-access and
>>> should thus go through the GC interface.
>>>
>>> So, how do others feel about this? Better to put this under
>>> Shenandoah-specific paths? Make it a proper GC/BarrierSet-interface? I
>>> also very much welcome suggestions for improving the suggested interface.
>> FWIW, I made that off-list comment. My objection is that:
>>
>>    666     if (BarrierSet::barrier_set()->barrier_set_assembler()->handle_cmpxchg_oop()) {
>>    667       tmp1 = new_register(T_OBJECT);
>>    668       tmp2 = new_register(T_OBJECT);
>>    669     }
>>
>> is shenandoah specific. I suppose you could have an API entry point that
>> would return some opaque object. That object for shenandoah would record
>> 2 extra registers. Then in the backend, you would pass that opaque
>> object to BarrierSetAssembler::cmpxchg_oop_c1() and the shenandoah
>> implementation would get its 2 extra registers.
>>
>> The problem is that with c2:
>>
>>   7471 instruct compareAndSwapP_BS(rRegI res,
>>   7472                             memory mem_ptr,
>>   7473                             rRegP tmp1, rRegP tmp2,
>>   7474                             rax_RegP oldval, rRegP newval,
>>   7475                             rFlagsReg cr)
>>   7476 %{
>>
>> is also shenandoah specific for the same reason (it needs 2 extra
>> registers). And I don't see a way to properly abstract that.
>>
>> So it seems to me, that code is not properly abstracted, there's no
>> reasonable way to properly abstract it, and it's actually shenandoah
>> specific code in disguise so it's better to have it be explicitly
>> shenandoah specific.
>>
>> Roland.
>>
>
> Thanks, Roland!
>
> So let me put my argument here too. The GC/BarrierSet-interface already
> has a couple of interfaces that are Shenandoah-specific. Basically all
> of what I added for supporting primitive access, object equality and
> allocations is Shenandoah-specific. There is a cmpxchg-oop interface in
> the runtime too. But then again, it is totally conceivable that a future
> GC might need those abstractions. In-fact, as far as I know, ZGC suffers
> from a similar problem with cmpxchg-oop as Shenandoah does, but there
> it's solved differently.
>
> Looking from a very-far-away perspective, one of the design ideas behind
> the current GC/BarrierSet-interface was that the GC should own all heap
> access. And cmpxchg-oop is quite definitely a heap-access. It is
> unfortunate that we can't seem to make it really clean with the way that
> the C2 backend is currently working. What I suggested seemed the closest
> we can get while remaining practical.
>
> Roman
>