RFR: 8361449: RISC-V: Code cleanup for native call [v2]
Feilong Jiang
fjiang at openjdk.org
Sat Jul 12 08:33:39 UTC 2025
On Mon, 7 Jul 2025 03:05:25 GMT, Dingli Zhang <dzhang at openjdk.org> wrote:
>> Hi, please consider this code cleanup change for native call.
>>
>> This removes the address parameter for NativeCall::reloc_destination and NativeFarCall::reloc_destination.
>> This also removes several unnecessary code blob related runtime checks turning them into assertions.
>>
>> ### Testing
>> * [x] hs:tier1 - hs:tier3 tested with linux-riscv64 fastdebug build
>
> Dingli Zhang has updated the pull request incrementally with one additional commit since the last revision:
>
> Remove outdated comments
src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 104:
> 102:
> 103: CodeBlob *code = CodeCache::find_blob(call_addr);
> 104: assert(code != nullptr && code->is_nmethod(), "nmethod expected");
The `if (code != nullptr && code->is_nmethod())` statement indicates that `code->is_nmethod()` may not always be true. Do you think we should use assert here? Looks like the new assert logic is inconsistent with the original code.
src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 105:
> 103: CodeBlob *code = CodeCache::find_blob(call_addr);
> 104: assert(code != nullptr && code->is_nmethod(), "nmethod expected");
> 105: nmethod* nm = code->as_nmethod();
`nm` is only used for `get_trampoline_for(call_addr, nm)`, maybe we can just use `code->as_nmethod()` directly instead of making a new variable.
src/hotspot/cpu/riscv/nativeInst_riscv.cpp line 156:
> 154: if (code->is_nmethod()) {
> 155: nmethod* nm = code->as_nmethod();
> 156: stub_addr = trampoline_stub_Relocation::get_trampoline_for(call_addr, nm);
Same here
Suggestion:
stub_addr = trampoline_stub_Relocation::get_trampoline_for(call_addr, code->as_nmethod());
src/hotspot/cpu/riscv/relocInfo_riscv.cpp line 76:
> 74: address Relocation::pd_call_destination(address orig_addr) {
> 75: assert(is_call(), "should be a call here");
> 76: if (orig_addr == nullptr) {
IIUC, it is synchronized with [JDK-8321509](https://bugs.openjdk.org/browse/JDK-8321509)? Should we add `USE_TRAMPOLINE_STUB_FIX_OWNER` and the other stuff?
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202426556
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202428678
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202431146
PR Review Comment: https://git.openjdk.org/jdk/pull/26150#discussion_r2202440479
More information about the hotspot-compiler-dev
mailing list