RFR: 8332689: RISC-V: Use load instead of trampolines [v11]

[Robbin Ehn]

On Fri, 14 Jun 2024 08:43:33 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits:
>> 
>>  - Merge branch 'master' into 8332689
>>  - Merge branch 'master' into 8332689
>>  - Merge branch 'master' into 8332689
>>  - Merge branch 'master' into 8332689
>>  - Remove tmp file
>>  - Prepare for dynamic NativeCall size
>>  - Only allow one calling convetion, i.e. fixed sized
>>  - Merge branch 'master' into 8332689
>>  - Review comments
>>  - Move shart/far code to cpp
>>  - ... and 6 more: https://git.openjdk.org/jdk/compare/5d2a19de...bb7249b8
>
> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 169:
> 
>> 167:   address addr = addr_at(0);
>> 168:   if (NativeShortCall::is_at(addr)) {
>> 169:     NativeShortCall* call = NativeShortCall::at(addr);
> 
> Are these lines necessary? As this is an instance method (rather than static), so `NativeShortCall::is_at(addr)` must already be true?

Yes as you can do:
`NativeCall* nc = (NativeCall*)(address);`

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 257:
> 
>> 255:     assert(!NativeShortCallTrampolineStub::is_at(dest), "chained trampolines");
>> 256:     NativeShortCallTrampolineStub::at(trampoline_stub_addr)->set_destination(dest);
>> 257:   }
> 
> Maybe move these lines into `else` block below? as `Assembler::reachable_from_branch_at(call_addr, dest)` condition check does not depends on these `trampoline_stub_addr` related check & set.

This is the original code, I have not changed it.

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 438:
> 
>> 436:   address stub_addr = trampoline_stub_Relocation::get_trampoline_for(call_addr, (nmethod*)code);
>> 437: 
>> 438:   if (stub_addr != nullptr) {
> 
> Could `stub_addr == nullptr`? If positive, then it should return false when it's nullptr, if negative, then should the `if` be converted to an `assert`?

Yes, it can, no.
Because we never want to execute anything more in `Relocation::pd_set_call_destination()` during NativeFarCall.
NativeShortCall (original) do a lot of weird things which seems a bit wrong/unnecessary.
I did not want to sort that out as I'm proposing not using it.

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 439:
> 
>> 437: 
>> 438:   if (stub_addr != nullptr) {
>> 439:     MacroAssembler::pd_patch_instruction_size(call_addr, stub_addr);
> 
> I could be wrong. `stub_addr` should be `dest`?

No, it is the stub address.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642416139
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642419489
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642412487
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642422227