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

[Roman Kennke]

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?");
>>>
>>>