[aarch64-port-dev ] RFR 8233339: Shenandoah: Centralize load barrier decisions into ShenandoahBarrierSet

[Zhengyu Gu]

Thanks for the review, Roman

-Zhengyu

On 11/7/19 2:41 PM, Roman Kennke wrote:
> That looks good to me.
> 
> Thanks,
> Roman
> 
>>>
>>> Filed: https://bugs.openjdk.java.net/browse/JDK-8233401
>>
>> Rebased on top of JDK-8233401
>>
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8233339/webrev.02/index.html
>>
>> Thanks,
>>
>> -Zhengyu
>>
>>
>>>
>>> Matter of fact, I would like to hold off this code review, till
>>> reactor is done.
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>>
>>>> *) shenandoahBarrierSetAssembler_x86.cpp, I believe it would be more
>>>> straightforward to save
>>>> branching on local variable "need_load_reference_barrier" by spelling
>>>> out the "disabled" path
>>>> directly (in fact, I think you are almost there in
>>>> shenandoahBarrierSetC1.cpp!):
>>>>
>>>>     if (!ShenandoahBarrierSet::need_load_reference_barrier(decorators,
>>>> type)) {
>>>>       BarrierSetAssembler::load_at(masm, decorators, type, dst, src,
>>>> tmp1, tmp_thread);
>>>>       return;
>>>>     }
>>>>
>>>>     ... code that assumes need_load_reference_barrier = true follows ...
>>>>
>>>>     Register result_dst = dst;
>>>>     bool use_tmp1_for_dst = false;
>>>>
>>>> *) shenandoahBarrierSetC1.cpp: local variable
>>>> "need_load_reference_barrier" is not needed, there is
>>>> only a single use
>>>>
>>>> *) shenandoahBarrierSetC2.cpp: this block should go all the way up:
>>>>
>>>>    557   if
>>>> (!ShenandoahBarrierSet::need_load_reference_barrier(decorators, type)) {
>>>>    558     return load;
>>>>    559   }
>>>>
>>>> *) shenandoahBarrierSet.cpp: this is just "return
>>>> is_reference_type(type)". Saves some inversions.
>>>>
>>>>     78   if (!is_reference_type(type)) return false;
>>>>     79   return true;
>>>>
>>>> *) shenandoahBarrierSet.cpp: should be "Should be subset of LRB":
>>>>
>>>>     83   assert(need_load_reference_barrier(decorators, type), "Why
>>>> ask?");
>>>>
>>>> *) shenandoahBarrierSet.cpp: seems like this assert is subsumed by
>>>> the previous one?
>>>>
>>>>      84   assert(is_reference_type(type), "Why we here?");
>>>>
>>>>
>