RFR: 8286301: Port JEP 425 to RISC-V

[Feilong Jiang]

On Mon, 31 Oct 2022 12:41:28 GMT, Fei Yang <fyang at openjdk.org> wrote:

> Hi,
> 
> Please review this PR porting JEP 425 (Virtual Threads) to RISC-V.
> 
> This is mainly adapted from the work of AArch64 port. Most of the changes lie in RISC-V scope.
> Changes to HotSpot shared code are trivial and are always guarded by RISCV64 macro. So this won't
> affect the rest of the world in theory.
> 
> There exists some differences in frame structure between AArch64 and RISC-V.
> For AArch64, we have:
> 
> enum {
>   link_offset           = 0,
>   return_addr_offset    = 1,
>   sender_sp_offset      = 2
> };
> 
> While for RISC-V, we have:
> 
> enum {
>   link_offset           = -2,
>   return_addr_offset    = -1,
>   sender_sp_offset      =  0
> };
> 
> So we need adapations in some places where the code relies on value of sender_sp_offset to work.
> Note that implementation for Post-call NOPs optimization is not incorporated in this PR as we plan to
> evaluate more on its impact on performance.
> 
> Testing on Linux-riscv64 HiFive Unmatched board:
>   - Minimal, Client and Server release & fastdebug build OK.
>   - Passed tier1-tier4 tests (release build).
>   - Passed jtreg tests under test/jdk/java/lang/Thread/virtual with extra JVM options: -XX:+VerifyContinuations -XX:+VerifyStack (fastdebug build).
>   - Performed benchmark tests like Dacapo, SPECjvm2008, SPECjbb2015, etc. to make sure no performance regression are introduced (release build).

Thanks for the great work! I have reviewed `cpu/riscv` part, and here are my comments.

src/hotspot/cpu/riscv/frame_riscv.inline.hpp line 388:

> 386:   }
> 387:   if (is_upcall_stub_frame()) {
> 388:     return sender_for_upcall_stub_frame(map);

looks like it's Foreign API related, do we need to introduce it in this pr?

src/hotspot/cpu/riscv/nativeInst_riscv.hpp line 558:

> 556: 
> 557: class NativePostCallNop: public NativeInstruction {
> 558: public:

could you add some comments for NativePostCallNop just like aarch64 did?

src/hotspot/cpu/riscv/riscv.ad line 2461:

> 2459:     __ bnez(flag, no_count);
> 2460: 
> 2461:     __ ld(tmp, Address(xthread, JavaThread::held_monitor_count_offset()));

can we just use `incremnet(Address(xthread, JavaThread::held_monitor_count_offset()))` here?

src/hotspot/cpu/riscv/riscv.ad line 2540:

> 2538:     __ bnez(flag, no_count);
> 2539: 
> 2540:     __ ld(tmp, Address(xthread, JavaThread::held_monitor_count_offset()));

`increment(Address)` can do the same thing.

src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp line 979:

> 977: 
> 978:     // Make sure the call is patchable
> 979:     __ align(NativeInstruction::instruction_size);

alignment was also done in `emit_trampoline_stub`, do we still need this `align` before emitting a trampoline call?

src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp line 1006:

> 1004: 
> 1005:   // Make sure the call is patchable
> 1006:   __ align(NativeInstruction::instruction_size);

ditto

src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp line 1030:

> 1028:   exception_offset = __ pc() - start;
> 1029:   {
> 1030:     __ mv(x9, x10); // save return value contaning the exception oop in callee-saved R9

maybe R9 -> x9 ?

src/hotspot/cpu/riscv/sharedRuntime_riscv.cpp line 1236:

> 1234:       guarantee(false, "Unknown Continuation native intrinsic");
> 1235:     }
> 1236: 

aarch64 has some assertions here, do we need this?

#ifdef ASSERT
    if (method->is_continuation_enter_intrinsic()) {
      assert(interpreted_entry_offset != -1, "Must be set");
      assert(exception_offset != -1,         "Must be set");
    } else {
      assert(interpreted_entry_offset == -1, "Must be unset");
      assert(exception_offset == -1,         "Must be unset");
    }
    assert(frame_complete != -1,    "Must be set");
    assert(stack_slots != -1,       "Must be set");
    assert(vep_offset != -1,        "Must be set");
#endif

src/hotspot/cpu/riscv/stubGenerator_riscv.cpp line 3879:

> 3877:       __ ld(c_rarg1, Address(fp, -1 * wordSize)); // return address
> 3878:       __ verify_oop(x10);
> 3879:       __ mv(x9, x10); // save return value contaning the exception oop in callee-saved R9

maybe R9 -> x9?

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

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