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

[Zhengyu Gu]

Hi Aleksey,

On 11/1/19 11:43 AM, Aleksey Shipilev wrote:
> On 11/1/19 3:15 PM, Zhengyu Gu wrote:
>> Webrev: http://cr.openjdk.java.net/~zgu/JDK-8233339/webrev.01/index.html
> 
> To be honest, it does not look like much of the improvement from the first glance. Maybe we should
> massage the code a bit to make it more readable? Roman also needs to take a look.

Right, it is not. But I believe that should be done in separate CR, as 
it may cause backport headache, right?

Filed: https://bugs.openjdk.java.net/browse/JDK-8233401

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