On Tue, 25 Aug 2026 07:49:00 GMT, Amit Kumar <[email protected]> wrote:
>> Harshit Dhiman has updated the pull request incrementally with one >> additional commit since the last revision: >> >> address review comments > > src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 64: > >> 62: >> 63: bool preserve_R2 = _result != Z_R2; >> 64: _nbytes_save = (15 - (preserve_R2 ? 0 : 1)) * BytesPerWord; > > shouldn't this be 12 + (0|1) ? > > R1 + R2(0 or 1) + R3-R5(3) + F0-F7(8) = 12+(0|1). Oh, I was saving space for R15 and R14 explicitly, but it is already included in bytes_save. I will update it. > src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 147: > >> 145: >> 146: Label done; >> 147: Label uncolor; > > Suggestion: > > NearLabel done, uncolor; There are a lot of places where we can use `NearLabel` instead of `Label` in this PR, I will test those and update them in a single commit. > src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 438: > >> 436: assert(zpointer != Z_ARG2, "or change argument setup"); >> 437: __ lgr_if_needed(Z_ARG2, addr); >> 438: __ >> call_VM_leaf(ZBarrierSetRuntime::load_barrier_on_oop_field_preloaded_addr(), >> zpointer, Z_ARG2); > > let call_VM_leaf handle the shuffling. > Suggestion: > > __ > call_VM_leaf(ZBarrierSetRuntime::load_barrier_on_oop_field_preloaded_addr(), > zpointer, addr); We can't do this here as `zpointer` and `addr` here is `R1` and `R2` respectively. Here we are loading `addr` into `Z_ARG2` first and then `call_VM_leaf` will load `zpointer` into `Z_ARG1`. But by default `call_VM_leaf` will try to put `zpointer` into `Z_ARG1` overwriting `addr`. (We will just hit a assert failure there) I can do it in a separate PR by changing the shuffling to a more robust implementation something like we have done in `ZSetupArguments` below. > src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 516: > >> 514: const Register zpointer = Z_R1; >> 515: Label done, loop, load_bad, load_good, store_bad, store_good; >> 516: __ z_slag(Z_R0, Z_ARG3, 3); > > should we zero-extend Z_ARG3 here ? because I am not sure if there exist a > guarantee that upper half will be zero or not. It's not guaranteed that upper half will be zero. But in the implementation for the non zGC version of this they are using an assert there. See `assert_positive_int` in https://github.com/openjdk/jdk/blob/c436633bb9be5854bc8c7eacdde3faae28aef3a9/src/hotspot/cpu/s390/stubGenerator_s390.cpp#L832 > src/hotspot/cpu/s390/gc/z/z_s390.ad line 198: > >> 196: match(Set newval (GetAndSetP mem newval)); >> 197: predicate(UseZGC && n->as_LoadStore()->barrier_data() != 0); >> 198: effect(TEMP temp1, TEMP temp2, KILL cr); > > `newval` is both input and output, I think RA should be aware of that. > Suggestion: > > effect(TEMP_DEF newval, TEMP temp1, TEMP temp2, KILL cr); RA is already aware of that as in match rule `newval` is an input to `GetAndSetP` and an output to the `Set`. This is also consistent with what x86 have done and in ppc the match rule itself is different from us. > src/hotspot/cpu/s390/jniFastGetField_s390.cpp line 41: > >> 39: #define __ masm-> >> 40: >> 41: #define BUFFER_SIZE 100*sizeof(jint) > > How did we came up with this number ? Are we following other architectures or > it's s390x specific value you came up with trial-and-error. This was trial-and-error but I mostly tried to keep to a nice round number. If you want this can be trimmed down. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3859857241 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3861453947 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3860052739 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3861613726 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3861427054 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3861476552
