RFR 8201786: Modularize interpreter GC barriers: leftovers for ARM32

[Erik Österlund]

Hi Aleksey,

I spoke too soon. I realize now that in do_oop_store/load, you have a 
decorator argument, but you never pass it along to load/store_heap_oop. 
That seems wrong. Now it seems like it works because you never pass in 
any decorators to do_oop_store/load. For example the array accesses have 
IN_HEAP_ARRAY, which causes card marking barriers to use precise marking 
instead. But it looks like they will never learn that this was indeed an 
array as the decorators are not passed along.

Thanks,
/Erik

On 2018-05-02 12:43, Erik Österlund wrote:
> Hi Aleksey,
>
> On 2018-05-02 11:21, Aleksey Shipilev wrote:
>> Thanks for review!
>>
>> Full:
>>    http://cr.openjdk.java.net/~shade/8201786/webrev.03/
>>
>> Changes from this review:
>>    http://cr.openjdk.java.net/~shade/8201786/webrev.03.diff
>>
>> Testing: arm32-hflt builds, adhoc runs with Serial/G1 -Xint on Raspi 3
>>
>> On 05/01/2018 07:10 PM, Erik Osterlund wrote:
>>> 1) The jobject resolve grabs the BarrierSetAssembler and calls 
>>> straight into it, instead of making
>>> an access_load_at call. I would prefer using the access call.
>> Right. Fixed.
>>
>>> 2) The macro assembler has a bunch of G1 includes left. I hope they 
>>> can be removed now.
>> Right. Removed.
>>
>>> 3) The CardTableBarrierSet checks for CMS flags instead of checking 
>>> whether the card table is
>>> scanned concurrently or not.
>> This is fixed separately, and you have reviewed this separately:
>>    https://bugs.openjdk.java.net/browse/JDK-8202418
>>
>>> 4) The template interpreter generator Reference.get intrinsic is 
>>> missing an ON_WEAK_OOP_REF
>>> decorator, which is required to get the right G1 barriers for example.
>> Good spot, fixed. After this change, assert_different_register 
>> started to fail with G1, so I had to
>> use the proper batch of temporary registers.
>>
>>> 5) In the template table, all do_oop_load/store accesses should be 
>>> IN_HEAP. They are missing that
>>> decorator. That will cause those accesses to skip pre/post write 
>>> barriers.
>> This is where I am confused. New ARM32 code in templateTable_arm.cpp 
>> repeats the shape spotted in
>> templateTable_x86.cpp. We call TemplateTable::do_oop_{load|store} 
>> with default decorators, sometimes
>> mixing in IN_HEAP_ARRAY. It then delegates into 
>> MacroAssembler::{load|store}_heap_oop, which mixes
>> in "IN_HEAP |".
>
> Right, I see now that you use load/store_heap_oop. For some reason I 
> read it as access_load/store_at.
>
> Your changes look good now. Thanks for sorting this out.
>
> /Erik
>
>>
>>> 6) I don’t know if you want to venture into the shady domain of jni 
>>> fast get field. If you do,
>>> the implicit jweak resolve in there on ARM should call
>>> BarrierSetAssembler::try_resolve_jobject_in_native. ... I filed
>>> https://bugs.openjdk.java.net/browse/JDK-8202480 for that. So might 
>>> be a different RFR I guess.
>> Yes, let it be separate RFR.
>>
>> -Aleksey
>>
>>
>