RFR(M): 8202713: Create a MacroAssembler::access_load/store_at wrapper for S390 and PPC

[Lindenmaier, Goetz]

Hi Martin, 

thanks a lot for your adaptions, looks good now.

Best regards,
  Goetz.

> -----Original Message-----
> From: Doerr, Martin
> Sent: Mittwoch, 16. Mai 2018 14:30
> To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>; Erik Österlund
> <erik.osterlund at oracle.com>; hotspot-runtime-dev at openjdk.java.net
> Subject: RE: RFR(M): 8202713: Create a
> MacroAssembler::access_load/store_at wrapper for S390 and PPC
> 
> Hi Götz,
> 
> > barrierSetAssembler_ppc.cpp
> >
> > The name of the label is_null is misleading. It sounds
> > as the name of a boolean variable. Maybe "L_handle_null"?
> > And please use explicit compare for pointers: if (is_null == NULL) {
> >
> Done.
> 
> > Shouldn't there be an assertion that no other decorators
> > are set than those handled here? Like all the memory ordering
> > ones?
> >
> Done.
> 
> > macroAssembler_ppc.hpp:
> >
> > This header uses DecoratorSet thus should #include accessDecorators.hpp
> >
> Done.
> 
> > access_load/store_at()
> > This is a misleading name for this function. I would think this
> > should go to nativeInst_ppc.cpp :)  Why would you access a store??
> > But this needs to be changed on all platforms. So not
> > scope of this review.
> > The function is not really needed. Make it private, or inline
> > it into load/store_heap_oop.
> >
> Made private.
> 
> > load_heap_oop_not_null:
> > Please remove this on ppc, too. It's the only platform
> > that still uses this function. There are only 6 uses.
> > Please also remove the dead definitions of it on aarch64 and x86,
> > as well as the comment in s390.ad mentioning it. (not sure about
> > aarch64 as you can't easily test this).
> >
> Changed PPC64 and s390 code to use the full featured version only.
> Removed old functions.
> I think it would be kind of unfair to change x86/aarch64 in this change
> because x86 folks will probably not read this review.
> 
> > Please extend the comment above load_heap_oop. It gained
> > new functionality, and after removing the _not_null functions
> > along with their documentation the current comment is pointless.
> > Mention what the label is good for. Rename the label as above.
> >
> Done.
> 
> > macroAssembler_ppc.inline.hpp
> >
> > Please rename the boolean variable as_raw to without_gc_barrier.
> > Actually, the DecoratorSet name AS_RAW is misleading, there is still
> > encode/decode etc.  But this is not in the scope of this change.
> >
> Not done because I'd like to keep PPC64 and s390 implementation consistent
> to the other platforms. A cleanup change for all platforms could be done in a
> separate change without s390 or PPC in its title.
> 
> > stubGenerator_ppc.cpp
> > Remove comment // Load the oop. Makes the line too long, and
> > not too much information gained :)
> >
> Done.
> 
> New webrev:
> http://cr.openjdk.java.net/~mdoerr/8202713_ppc64_s390_masm_access/w
> ebrev.01/
> 
> Best regards,
> Martin