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

[Roman Kennke]

Hello all,

submit came back with failures again. See below. From afar it looks like
the same stuff caused by JDK-8203780 that made the tests for
"JDK-8202776: Modularize GC allocations in runtime" fail. Can you please
confirm that it's ok to push (or not)?

Thanks, Roman


> Hi Roman,
> 
> Looks good.
> 
> Thanks,
> /Erik
> 
> On 2018-06-05 18:29, Roman Kennke wrote:
>> Am 05.06.2018 um 17:33 schrieb 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.
>>>
>> Ok I've added it:
>>
>> Incremental:
>> http://cr.openjdk.java.net/~rkennke/JDK-8200623/webrev.01.diff/
>> Full:
>> http://cr.openjdk.java.net/~rkennke/JDK-8200623/webrev.01/
>>
>> Good now?
>>
>> Thanks, Roman
>>
>