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

[Aleksey Shipilev]

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.

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


-- 
Thanks,
-Aleksey