RFR: 8288971: AArch64: Clean up stack and register handling in interpreter

[Andrew Dinn]

On Wed, 22 Jun 2022 13:00:44 GMT, Andrew Haley <aph at openjdk.org> wrote:

> There are several places in the interpreter that could be improved.
> 
> 1. We use r13 to pass the caller's SP to a callee through adapters. r13 is not a callee-saved register in the native ABI, so this causes some complications. Use a callee-saved register.
> 2. We frequently recalculate the location where the native SP needs to go. We have a spare slot in the interpreter frame, so we should calculate it once, when the frame is created, and use it.
> 3. Related to 1, we should clearly label all the places where the caller's SP is passed to a callee.

A little bit of rework needed.

src/hotspot/cpu/aarch64/abstractInterpreter_aarch64.cpp line 176:

> 174:   int max_stack = method->constMethod()->max_stack() + MAX2(3, Method::extra_stack_entries());
> 175:   intptr_t* extended_sp = (intptr_t*) monbot  -
> 176:     (max_stack * Interpreter::stackElementWords) -

You are correctly multiplying by Interpreter::stackElementWords here. However, I noticed that at line 93 the multiply has been omitted

  int size = overhead +
         (callee_locals - callee_params) +
         monitors * frame::interpreter_frame_monitor_size() +

It should really follow x86 and include the multiply:

  int size = overhead +
         (callee_locals - callee_params) * Interpreter::stackElementWords +
         monitors * frame::interpreter_frame_monitor_size() +

We only get away with this because Interpreter::stackElementWords is 1.

Perhaps you could correct that as part of this patch?

src/hotspot/cpu/aarch64/abstractInterpreter_aarch64.cpp line 183:

> 181:   // All frames but the initial (oldest) interpreter frame we fill in have
> 182:   // a value for sender_sp that allows walking the stack but isn't
> 183:   // truly correct. Correct the value here.

The condition this comment leads into tests for

interpreter_frame->sender_sp() == interpreter_frame->interpreter_frame_sender_sp())

I'm not really clear how/where that actually happens? Can you explain?

src/hotspot/cpu/aarch64/frame_aarch64.cpp line 607:

> 605:     DESCRIBE_FP_OFFSET(interpreter_frame_method);
> 606:     DESCRIBE_FP_OFFSET(interpreter_frame_mdp);
> 607:     DESCRIBE_FP_OFFSET(interpreter_frame_extended_sp);

Nothing wrong with adding this here but I notice you have not added `DESCRIBE_FP_OFFSET(interpreter_frame_extended_sp)` at line 679 in function `internal_pf()`. That function also fails to include `DESCRIBE_FP_OFFSET(interpreter_frame_mirror)`. Is this intended or accidental?

src/hotspot/cpu/aarch64/methodHandles_aarch64.cpp line 188:

> 186:   }
> 187: 
> 188:   // r19_sender_sp: sender SP (must preserve; see prepare_to_jump_from_interpreted)

There are two other references to r13 that either need removing or fixing

286   Register temp3 = r14;  // r13 is live by this point: it contains the sender SP

and

356     // Live registers at this point:
357     //  member_reg - MemberName that was the trailing argument
358     //  temp1_recv_klass - klass of stacked receiver, if needed
359     //  r13 - interpreter linkage (if interpreted)  ??? FIXME
360     //  r1 ... r0 - compiler arguments (if compiled)

src/hotspot/cpu/aarch64/templateInterpreterGenerator_aarch64.cpp line 204:

> 202:     __ ldrd(v0, Address(esp, 2 * Interpreter::stackElementSize));
> 203:     __ ldrd(v1, Address(esp));
> 204:     __ mov(sp, r19_sender_sp);

Ok, finally a real bug! At line 200 we have

 200     __ mov(r19, lr);   <== this destroys sp in r19_sender_sp
 201     continuation = r19;


So, when you switch this from using `r13` to using `r19` here at line 204 you are effectively updating `sp` with `lr`.

Note this doesn't happen in the previous cases at line 193 because the assignments were done in the opposite order.

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

Changes requested by adinn (Reviewer).

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