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

[Fei Yang]

On Thu, 2 May 2024 23:17:45 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> 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 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?
>
>> 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.

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

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