RFR: 8294729: [s390] Implement nmethod entry barriers

[Tyler Steele]

On Tue, 11 Oct 2022 16:17:29 GMT, Lutz Schmidt <lucy 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.
>
> 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.

Good catch! I have changed this.

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

Thanks for explaining this. I always felt like there was a structure here, but wasn't sure the rules. Knowing about the CPU_HEADER macros is also especially useful.

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

This change is reasonable. Thanks for the suggestion.

> 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]);`

I like this. I have made this change.

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

I changed to the preferred test instruction. However, it seems like z_brnz won't work in this situation because the branch address is in a register and not a label.

Side note it too me a second to realize that I am using bcr and brnz is an alias for brc. It seems odd that there isn't a "speaking" alias for bcr (or maybe I'm just not finding it).

> 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?

You're right; this is dead code. Thanks.

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

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