RFR: 8294729: [s390] Implement nmethod entry barriers

[Lutz Schmidt]

On Tue, 4 Oct 2022 14:27:09 GMT, Tyler Steele <tsteele at openjdk.org> wrote:

> This draft PR implements native method barriers on s390. When complete, this will fix the build, and bring the other benefits of [JDK-8290025](https://bugs.openjdk.org/browse/JDK-8290025) to that platform.

Changes requested by lucy (Reviewer).

Changes requested by lucy (Reviewer).

src/hotspot/cpu/s390/c1_MacroAssembler_s390.cpp line 79:

> 77: 
> 78:   BarrierSetAssembler* bs = BarrierSet::barrier_set()->barrier_set_assembler();
> 79:   bs->nmethod_entry_barrier(this, Z_R3);

Ha! Another use of Z_R3? Is it safe here? Probably not.

In s390 code, you can only use Z_R0_scratch and Z_R1_scratch safely as scratch registers. Because everybody codes with that knowledge, you can't rely on these scratch registers to be preserved across generator calls - except when you have the entire call chain under your control.

When you pass one of the scratch registers as tmp register to a generator, you have to know how the tmp is used. When it's used in address calculation or to hold the address of something, you can't pass Z_R0_scratch. You for sure know that.

src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp line 27:

> 25: 
> 26: #include "precompiled.hpp"
> 27: #include "asm/macroAssembler.hpp"

This `#include` is unnecessary. It comes in via
`#include "asm/macroAssembler.inline.hpp"`
These are the rules for dependent includes:
 - if you need an `<arch>`-specific `<basename>.inline.hpp`, include the shared code counterpart instead. This will in turn include all the prereqs, especially `<basename>.hpp`, and then include the `<arch>`-specific `inline.hpp` via `CPU_HEADER_INLINE(<basename>)`.
 - if you need an `<arch>`-specific `<basename>.hpp`, include the shared code counterpart instead. This will in turn include all the prereqs and then include the `<arch>`-specific `.hpp` via `CPU_HEADER(<basename>)`.
 - Note that these rules are unfortunately not followed everywhere, leading to a lot of confusion and the insertion of arbitrary `#include` statements "until the thing builds".

src/hotspot/cpu/s390/gc/shared/barrierSetAssembler_s390.cpp line 93:

> 91:     // Compare to current patched value:
> 92:     __ z_cfi(tmp, /* to be patched */ -1); // 6 bytes (2 + 4 byte imm val)
> 93: 

What about the value at `thread_disarmed_offset()`? Is it 4 bytes or 8 bytes? If it is 4 bytes, you should load it with
`z_l(tmp, ...);` or 
`z_ly(tmp, ...);` if the offset might be negative.
Similar size considerations on the compare:
`z_cfi(tmp, ...);` is good for 4-byte value.
`z_cgfi(tmp, ...);` is good for 8-byte values. Caution: signed compare. Sign-extension of the immediate may produce unexpected behaviour.

src/hotspot/cpu/s390/gc/shared/barrierSetNMethod_s390.cpp line 64:

> 62:     }
> 63: 
> 64:     void verify() const {

Shouldn't the complete function body be encapsulated with 
#ifdef ASSERT
If ASSERT is undefined, the code has no externally visible effect and thus is irrelevant.

src/hotspot/cpu/s390/gc/shared/barrierSetNMethod_s390.cpp line 81:

> 79:       assert(Assembler::is_equal(start[offset], BCR_ZOPC, RIL_MASK), "check BCR");
> 80:       offset += Assembler::instr_len((unsigned long)BCR_ZOPC);
> 81: 

I don't like specifying the same information (here: instruction opcode) multiple times. I would suggest you increment the offset with code like
`offset += Assembler::instr_len(&start[offset]);`

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 2891:

> 2889:     __ z_cfi(Z_R2, 0);
> 2890:     __ z_bcr(Assembler::bcondNotEqual, Z_R14);
> 2891: 

`z_ltr(Z_R2, Z_R2);`
is the preferred way of testing register contents. I would then use the "speaking" alias
`z_brnz(Z_R14);
Note: there are subtle semantic differences between "not equal" and "not zero". See assembler_s390.hpp.

src/hotspot/cpu/s390/stubGenerator_s390.cpp line 2899:

> 2897:     // Call wrong-method-stub
> 2898:     __ z_br(Z_R2);
> 2899: 

This is dead code.
Or should the branch above be a call instead?

-------------

PR: https://git.openjdk.org/jdk/pull/10558