RFR: 8326306: RISC-V: Re-structure MASM calls and jumps [v2]

[Robbin Ehn]

On Mon, 6 May 2024 06:18:45 GMT, Fei Yang <fyang at openjdk.org> wrote:

>>> We still need relocates rt_call, not sure why you removed it.
>> 
>> I removed the relocate because I am thinking it should be absolute calls in the else block of `MacroAssembler::rt_call` [1], so no reloc required, as you mentioned in your previous comments. 
>> 
>> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L5027
>> 
>>> It seem like we need two version of rt_call one with address and one with Address. Then it seem like we could remove far_call as the rt_call would do the right thing.
>>> 
>>> I like your idea, and we should do that, but it seems like it's not trivial just to add to this patch. Is there a reason we need to include such in changes in this PR?
>> 
>> On holiday this week, we can discuss further next week :-)
>
>> We still need relocates rt_call, not sure why you removed it. It seem like we need two version of rt_call one with address and one with Address. Then it seem like we could remove far_call as the rt_call would do the right thing.
> 
> I guess maybe it's easier to only consider `call` and `rt_call` as a first step? And I prefer to keep `far_call` and `far_jump` as I think they are kind of different from rt_call. These two only handle the case when target is within code cache [1][2] as compared to rt_call which handles both code-cache and non-code-cache targets.
> 
> [1] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L3173
> [2] https://github.com/openjdk/jdk/blob/master/src/hotspot/cpu/riscv/macroAssembler_riscv.cpp#L3188
> 
>> I like your idea, and we should do that, but it seems like it's not trivial just to add to this patch. Is there a reason we need to include such in changes in this PR?
> 
> Another concern which also leads me to consider unifying `call` and `rt_call` is possible performance issues. I see some `call` are changed into `rt_call` in this PR like in file stubGenerator_riscv.cpp and templateInterpreterGenerator_riscv.cpp. As I mentioned in my previous comments, `rt_call` would emit a fixed-size `movptr` sequence (6 uncompressed instructions) for these call sites which invokes some C++ VM functions. But it's still possible for the original `call` to emit a more simpler auipc + jalr depending on the `is_32bit_offset_from_codecache` check in `la`, right? Hopefully, we can get rid of this issue with my proposed change.

Yes, sorry I didn't explain that:
The cases in stubGenerator are all slow-path so we don't care much about performance.
Calling:
- MacroAssembler::debug64 debug
- JavaThread::check_special_condition_for_native_trans when suspend flag is set
- SharedRuntime::reguard_yellow_pages regaurd pages => syscall
- Interpreter::trace_code(t->tos_in()) Calling a interpreter trace method for the bytcode
- __ rt_call(runtime_entry); calling runtime when throwing an exception

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18942#discussion_r1590595509