RFR: JDK-8200623: Primitive heap access for interpreter BarrierSetAssembler/x86

[Erik Österlund]

Hi Roman,

On 2018-06-05 16:07, Roman Kennke wrote:
> Hi Erik,
>
>>> JDK-8199417 added better modularization for interpreter barriers.
>>> Shenandoah and possibly future GCs also need barriers for primitive
>>> access.
>>>
>>> Some notes on implementation:
>>> - float/double/long access produced some headaches for the following
>>> reasons:
>>>
>>>     - float and double would either take XMMRegister which is not
>>> compatible with Register
>>>     - or load-from/store-to the floating point stack (see
>>> MacroAssembler::load/store_float/double)
>>>     - long access on x86_32 would load-into/store-from 2 registers, or
>>> else use a trick via the floating point stack to do atomic access
>>>
>>> None of this seemed easy/nice to do with the API. I helped myself by
>>> accepting noreg as dst/src argument, which means the corresponding tos
>>> (i.e. ltos, ftos, dtos) and the BSA would then access from/to
>>> xmm0/float-stack in case of float/double or the double-reg/float-stack
>>> in case of long/32bit, which is all that we ever need.
>> It is indeed a bit painful that in hotspot, XMMRegister is not a
>> Register (unlike the Graal implementation). And I think I agree that if
>> it is indeed only ever needed by ToS, then this is the preferable
>> solution to having two almost identicaly APIs - one for integral types
>> and one for floating point types. It beats me though, that in this patch
>> you do not address the jni fast get field optimization on x86. It is
>> seemingly missing barriers now. Should probably make sure that one fits
>> in as well. Fortunately, I think it should work out pretty well.
> As mentioned in the review thread for JDK-8203172, we in Shenandoah land
> decided to disable JNI fastgetfield stuff for now. I am not sure whether
> or not we want to go through access_* anyway? It's probably more
> consistent if we do. If we decide that we do, I'll add it to this patch,
> if we don't, I'll rip it out of JDK-8203172 :-)

Okay so let's not deal with JNI fast get field in either of those these 
two patches, and leave that exercise to future adventurers that feel 
brave enough to change that code. If you decide later to add 
modularization for that to enable this fantastic performance 
optimization on Shenandoah, then perhaps we can have a new patch with 
the appropriate code (possibly the speculative PC range filter I 
proposed) then in the new modularization.

In that case, it looks reasonable the way it is now. Perhaps there 
should be a T_ADDRESS case for stores for consistency though? You can 
now load a T_ADDRESS but not store it, which is a bit surprising I 
suppose. Otherwise it looks good.

Thanks,
/Erik

> Thanks, Roman
>