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

[Robbin Ehn]

On Fri, 14 Jun 2024 10:07:34 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 198:
> 
>> 196:   Assembler::patch(pInsn, 30, 21, (offset >> 1) & 0x3ff);
>> 197:   Assembler::patch(pInsn, 20, 20, (offset >> 11) & 0x1);
>> 198:   Assembler::patch(pInsn, 19, 12, (offset >> 12) & 0xff);
> 
> should we reuse `MacroAssembler::pd_patch_instruction_size`?

This is the original code, I have not change it. (just moved from hpp -> cpp)

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 248:
> 
>> 246: }
>> 247: 
>> 248: bool NativeShortCall::reloc_set_destination(address dest) {
> 
> `reloc_set_destination` and `set_destination_mt_safe` are almost same, maybe `set_destination_mt_safe` could call `reloc_set_destination`?

Ok

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 410:
> 
>> 408: }
>> 409: 
>> 410: bool NativeFarCall::set_destination_mt_safe(address dest, bool assert_lock) {
> 
> Seems no caller will pass `assert_lock  == false`

No, but this is the interface in NativeCall which just delegates here.

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 410:
> 
>> 408: }
>> 409: 
>> 410: bool NativeFarCall::set_destination_mt_safe(address dest, bool assert_lock) {
> 
> For NativeShortCall, reloc_set_destination and set_destination_mt_safe are almost same, but for NativeFarCall they're different, is this expected?

They are not the same case, but the original code tries to handle them in the same method which just makes the code complicated.

> src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 423:
> 
>> 421: 
>> 422:   if (stub_addr != nullptr) {
>> 423:     set_stub_address_destination_at(stub_addr, dest);
> 
> Is `ICache::invalidate_range` needed here?

No as we do a data store and data read from this location.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642426941
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642432228
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642429501
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642434302
PR Review Comment: https://git.openjdk.org/jdk/pull/19453#discussion_r1642430372