Skip to content

8350182: [s390x] Relativize locals in interpreter frames #23660

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

8350182: [s390x] Relativize locals in interpreter frames #23660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
018d5bc
locals relativization on s390x
offamitkumar Feb 17, 2025
46d6ae1
use Z_R0 as helper
offamitkumar Feb 25, 2025
e36c498
use Z_R0 as temp
offamitkumar Apr 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/hotspot/cpu/s390/frame_s390.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ bool frame::is_interpreted_frame() const {

void frame::interpreter_frame_set_locals(intptr_t* locs) {
assert(is_interpreted_frame(), "interpreted frame expected");
ijava_state_unchecked()->locals = (uint64_t)locs;
// set relativized locals
*addr_at(_z_ijava_idx(locals)) = (intptr_t) (locs - fp());
}

// sender_sp
Expand Down Expand Up @@ -340,7 +341,7 @@ bool frame::is_interpreted_frame_valid(JavaThread* thread) const {
if (MetaspaceObj::is_valid(cp) == false) return false;

// validate locals
address locals = (address)(ijava_state_unchecked()->locals);
address locals = (address)interpreter_frame_locals();
return thread->is_in_stack_range_incl(locals, (address)fp());
}

Expand Down
6 changes: 5 additions & 1 deletion src/hotspot/cpu/s390/frame_s390.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2016, 2024 SAP SE. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -330,6 +330,10 @@
#define _z_ijava_state_neg(_component) \
(int) (-frame::z_ijava_state_size + offset_of(frame::z_ijava_state, _component))

// Frame slot index relative to fp
#define _z_ijava_idx(_component) \
(_z_ijava_state_neg(_component) >> LogBytesPerWord)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a function?
Also, names starting with _ aren't common in HotSpot code, except for fields in C++ objects.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a function?

This is similar to ppc ijava_idx

Also, names starting with _ aren't common in HotSpot code, except for fields in C++ objects.

I did it for keeping the parity with:

#define _z_ijava_state_neg(_component) \
         (int) (-frame::z_ijava_state_size + offset_of(frame::z_ijava_state, _component))

Do you want me to revert it ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no good reason to use a macro here. If a function can be a function, and this one can, let it be one. However, there is no reason to change anything else. Leave that for a "macros to functions" patch some other day.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could only achieve this implementation: macro to method, which looks dirty.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough. It's not worth doing that, and what you do here does match PPC. OK.

// ENTRY_FRAME

struct z_entry_frame_locals {
Expand Down
5 changes: 3 additions & 2 deletions src/hotspot/cpu/s390/frame_s390.inline.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2016, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2016, 2025, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2016, 2024 SAP SE. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
Expand Down Expand Up @@ -180,7 +180,8 @@ inline intptr_t* frame::link_or_null() const {
}

inline intptr_t* frame::interpreter_frame_locals() const {
return (intptr_t*) (ijava_state()->locals);
intptr_t n = *addr_at(_z_ijava_idx(locals));
return &fp()[n]; // return relativized locals
}

inline intptr_t* frame::interpreter_frame_bcp_addr() const {
Expand Down
12 changes: 11 additions & 1 deletion src/hotspot/cpu/s390/interp_masm_s390.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,15 @@ void InterpreterMacroAssembler::dispatch_base(TosState state, address* table, bo
}
{ Label OK;
// check if the locals pointer in Z_locals is correct
z_cg(Z_locals, _z_ijava_state_neg(locals), Z_fp);

// _z_ijava_state_neg(locals)) is fp relativized, so we need to
// extract the pointer.

z_lg(Z_R1_scratch, Address(Z_fp, _z_ijava_state_neg(locals)));
z_sllg(Z_R1_scratch, Z_R1_scratch, Interpreter::logStackElementSize);
z_agr(Z_R1_scratch, Z_fp);

z_cgr(Z_locals, Z_R1_scratch);
Comment on lines -109 to +117
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By default assertion was failing, Because stored value is fp relativised and Z_locals is holding pointer. So I have updated it.

I have done some manual step through and found that it should be okay to keep, unless we don't want to kill another register here.

diff --git a/src/hotspot/cpu/s390/templateInterpreterGenerator_s390.cpp b/src/hotspot/cpu/s390/templateInterpreterGenerator_s390.cpp
index c40be5edec7..fc5b9f10af1 100644
--- a/src/hotspot/cpu/s390/templateInterpreterGenerator_s390.cpp
+++ b/src/hotspot/cpu/s390/templateInterpreterGenerator_s390.cpp
@@ -1134,8 +1134,16 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
   __ z_agr(Z_locals, Z_esp);
   // z_ijava_state->locals - i*BytesPerWord points to i-th Java local (i starts at 0)
   // z_ijava_state->locals = Z_esp + parameter_count bytes

+  __ z_sgrk(Z_locals, Z_locals, fp); // Z_R1 = Z_locals - fp();
+  __ z_srlg(Z_locals, Z_locals, Interpreter::logStackElementSize);
+  // Store relativized Z_locals, see frame::interpreter_frame_locals().
   __ z_stg(Z_locals, _z_ijava_state_neg(locals), fp);
   // z_ijava_state->oop_temp = nullptr;
   __ store_const(Address(fp, oop_tmp_offset), 0);

with this change, Z_locals is holding the correct value (fp relativized) which is stored at the state offset:

(gdb) i r r12 
r12            0x1                 1
(gdb) x/2gx $r9 - 88 
0x3fffb37be98:	0x0000000000000001	0x000003fffb37be80
(gdb)

Similarly, with the change I have pushed:

instr:

0x3fff8192b5a:	lg	%r1,-88(%r9)
0x3fff8192b60:	sllg	%r1,%r1,3
0x3fff8192b66:	agr	%r1,%r9


result: 

(gdb) i r r12 
r12            0x3fffb37bef8       4397966278392
(gdb) i r r1 
r1             0x3fffb37bef8       4397966278392
(gdb)

z_bre(OK);
reentry = stop_chain_static(reentry, "invalid locals pointer Z_locals: " FILE_AND_LINE);
bind(OK);
Expand Down Expand Up @@ -686,6 +694,8 @@ void InterpreterMacroAssembler::save_mdp(Register mdp) {
void InterpreterMacroAssembler::restore_locals() {
asm_assert_ijava_state_magic(Z_locals);
z_lg(Z_locals, Address(Z_fp, _z_ijava_state_neg(locals)));
z_sllg(Z_locals, Z_locals, Interpreter::logStackElementSize);
z_agr(Z_locals, Z_fp);
}

void InterpreterMacroAssembler::get_method(Register reg) {
Expand Down
6 changes: 5 additions & 1 deletion src/hotspot/cpu/s390/templateInterpreterGenerator_s390.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,7 +1134,11 @@ void TemplateInterpreterGenerator::generate_fixed_frame(bool native_call) {
__ z_agr(Z_locals, Z_esp);
// z_ijava_state->locals - i*BytesPerWord points to i-th Java local (i starts at 0)
// z_ijava_state->locals = Z_esp + parameter_count bytes
__ z_stg(Z_locals, _z_ijava_state_neg(locals), fp);

__ z_sgrk(Z_R0, Z_locals, fp); // Z_R0 = Z_locals - fp();
__ z_srlg(Z_R0, Z_R0, Interpreter::logStackElementSize);
// Store relativized Z_locals, see frame::interpreter_frame_locals().
__ z_stg(Z_R0, _z_ijava_state_neg(locals), fp);

// z_ijava_state->oop_temp = nullptr;
__ store_const(Address(fp, oop_tmp_offset), 0);
Expand Down