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

[jesper.wilhelmsson at oracle.com]

Yes, all failures in 2018-06-05-1735591.roman.source are due to JDK-8203780 (or I should probably say JDK-8204321 which is the bug to handle the issue).
/Jesper

> On 5 Jun 2018, at 20:48, Roman Kennke <rkennke at redhat.com> wrote:
> 
> 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
>>> 
>> 
> 
>