On Tue, 25 Aug 2026 06:40:47 GMT, Harshit Dhiman <[email protected]> wrote:
>> Port the Z garbage collector to s390. >> >> The tier1 test case are passing using `-XX:+UseZGC` flag >> >> --------- >> - [x] I confirm that I make this contribution in accordance with the >> [OpenJDK Interim AI Policy](https://openjdk.org/legal/ai). > > 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/zAddress_s390.cpp line 37: > 35: #include <sys/mman.h> > 36: #endif // LINUX > 37: Suggestion: 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). src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 68: > 66: > 67: __ push_frame_abi160(_nbytes_save); offset += 8; > 68: __ save_return_pc(); offset += 8; you are saving R14 into SP+8, not to the slot you got from _nbyte_save. So I don't think you should advance offset it here. src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 96: > 94: > 95: __ restore_return_pc(); offset += 8; > 96: __ z_lg(Z_R1, offset, Z_SP); offset += 8; same as above. src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 147: > 145: > 146: Label done; > 147: Label uncolor; Suggestion: NearLabel done, uncolor; src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 153: > 151: // > 152: > 153: // Load adress Suggestion: // Load address src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 258: > 256: __ bind(medium_path_continuation); > 257: if (rnew_zaddress == noreg) { > 258: __ z_xgr(rnew_zpointer, rnew_zpointer); Suggestion: __ clear_reg(rnew_zpointer, true, false); src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 261: > 259: } else { > 260: __ z_lgr(rnew_zpointer, rnew_zaddress); > 261: __ z_sllg(rnew_zpointer, rnew_zpointer, ZPointerLoadShift); Can we do this ? Suggestion: __ z_sllg(rnew_zpointer, rnew_zaddress, ZPointerLoadShift); src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 370: > 368: } > 369: __ z_sllg(temp1, temp1, ZPointerLoadShift); > 370: __ z_og(temp1, Address(Z_thread, > ZThreadLocalData::store_good_mask_offset())); Wouldn't this one will be better ? When temp1 holds zero, then what's the point of doing a shift operation on it ? Suggestion: if (src == noreg) { __ clear_reg(temp1, true, false); } else { __ z_sllg(temp1, src, ZPointerLoadShift); } __ z_og(temp1, Address(Z_thread, ZThreadLocalData::store_good_mask_offset())); 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); src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 469: > 467: Label& continuation, > 468: bool dest_unintialized) > const { > 469: if (!dest_unintialized) { Suggestion: bool dest_uninitialized) const { if (!dest_uninitialized) { src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 490: > 488: const Register zpointer = Z_R1; > 489: Label done, loop, load_bad, load_good, store_bad, store_good; > 490: __ z_cghi(Z_ARG3, 0); go for `z_ltgr` that's 2 byte instruction. 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. src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.cpp line 645: > 643: __ z_lgr(temp, obj); > 644: __ relocate(barrier_Relocation::spec(), > ZBarrierRelocationFormatMarkBadBeforeTest); > 645: __ z_nill(temp, barrier_Relocation::unpatched); you can use tmll here also. src/hotspot/cpu/s390/gc/z/zBarrierSetAssembler_s390.hpp line 125: > 123: void generate_disjoint_oop_copy(MacroAssembler* masm, bool > dest_unintialized); > 124: > 125: void generate_conjoint_oop_copy(MacroAssembler* masm, bool > dest_unintialized); Suggestion: bool dest_uninitialized) const; void generate_disjoint_oop_copy(MacroAssembler* masm, bool dest_uninitialized); void generate_conjoint_oop_copy(MacroAssembler* masm, bool dest_uninitialized); 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); ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850351849 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850633065 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850641047 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850642463 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850655298 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850409092 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850712564 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850441628 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850526903 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3851016591 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3851028287 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3851112397 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3851185627 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3851513815 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3851041757 PR Review Comment: https://git.openjdk.org/jdk/pull/31984#discussion_r3850297412
