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

[Lindenmaier, Goetz]

Hi Martin, 

I had a look at your code.

This also is the first time I look at the new barrier handling.
I find it confusing this is called "barrier". It handles not 
only the barriers but does the accesses, too. Thus I would
have called the whole functionality OopAccessAssembler or
JavaHeapAccessAssembler.
Why should I call BarrierSetAssembler::load_at if I want to load
a value from the heap? It would read better as OopAccessAssembler::load.

Further, I miss some comment of what the changed functions are doing,
e.g. for BarrierSetAssembler::load_at():
"Generate code to load a value of Java type "type" from address 
<base+ind_or_offs> into register dst.  Issue gc barriers, memory ordering,
oop compression, null handling etc. acording to decorators."
The functions are quite complex, thus a bit of comment would
help. The documentation should be similar on all platforms.

Also, I don't like the directory structure in the cpu
directory. There are no subdirectories for interpreter, c1, c2
etc. either.

But this is not subject of your change.

Detailed review:

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) {

Shouldn't there be an assertion that no other decorators
are set than those handled here? Like all the memory ordering
ones?

macroAssembler_ppc.hpp:

This header uses DecoratorSet thus should #include accessDecorators.hpp

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.

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).

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.

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.

stubGenerator_ppc.cpp
Remove comment // Load the oop. Makes the line too long, and 
not too much information gained :)

Similar comments hold for the s390 case.

Best regards,
   Goetz.


> -----Original Message-----
> From: hotspot-runtime-dev [mailto:hotspot-runtime-dev-
> bounces at openjdk.java.net] On Behalf Of Doerr, Martin
> Sent: Freitag, 11. Mai 2018 12:43
> To: Erik Österlund <erik.osterlund at oracle.com>; hotspot-runtime-
> dev at openjdk.java.net
> Subject: [CAUTION] RFR(M): 8202713: Create a
> MacroAssembler::access_load/store_at wrapper for S390 and PPC
> 
> Hi Erik,
> 
> thanks for creating the bug. It makes sense to use load/store_heap_oop like
> on the other platforms.
> It will especially be useful when we add support for a GC with load barriers.
> 
> Here's the webrev:
> http://cr.openjdk.java.net/~mdoerr/8202713_ppc64_s390_masm_access/w
> ebrev.00/
> 
> Please review.
> 
> Best regards,
> Martin