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

[Fei Yang]

On Wed, 8 May 2024 15:07:09 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi, please consider.
>> 
>> We have code that directly use the asm for call/jumps instead masm.
>> Our masm have a bit odd naming, and we don't use 'proper' pseudoinstructions/mnemonics.
>> Suggested by [riscv-asm-manual](https://github.com/riscv-non-isa/riscv-asm-manual/tree/master)
>> 
>> j offset	jal x0, offset	Jump
>> jal offset	jal x1, offset	Jump and link
>> jr rs	        jalr x0, rs, 0	Jump register
>> jalr rs	        jalr x1, rs, 0	Jump and link register
>> ret	        jalr x0, x1, 0	Return from subroutine
>> call offset	auipc x1, offset[31:12]; jalr x1, x1, offset[11:0]	Call far-away subroutine	
>> tail offset	auipc x6, offset[31:12]; jalr x0, x6, offset[11:0]	Tail call far-away subroutine
>> 
>> But these can only be implemented like this if you have small enough application.
>> The fallback of these is to use GOT (your C compiler should place a copy of GOT every 2G so it's always reachable).
>> We don't have GOT, instead we materialize, so there is still differences between these and ours.
>> 
>> This patch:
>> - Tries to follow these suggested mappings as good we can.
>> - Make sure all jumps/calls go through MASM. (so we get control and can easily change for sites using a certain calling convention)
>> - To avoid confusion between MASM public/private methods and ASM methods and the mnemonics there are some renaming.
>>   E.g. the mnemonics jal means call offset, as we can't use that so there is no 'jal'.
>> - I enabled c.j, but right now we never generate it.
>> - As always the macro does no good and are legacy from when code base did not use templates. (also the x-macros screws up my IDE (vim+rtags))
>> 
>> I started down this path due to I have followup patch on top of this which removes trampoline in favor for load-n-jump.
>> (WIP: https://github.com/robehn/jdk/compare/jal-fixes...robehn:jdk:load-n-link?expand=1)
>> While looking into our calls it was a bit confusing, this helps. 
>> 
>> Done a couple of t1-3 slightly different version of this patch, and as part of the followup, no issues found. (VF2, qemu, LP4)
>> Re-running tests, had some last minute changes.
>> 
>> Thanks, Robbin
>
> Robbin Ehn has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision:
> 
>  - Merge branch 'master' into jal-fixes
>  - Revert JNI field, call()->li()
>  - Use li instead of movptr for call
>  - REVERT: Use li instead of movptr
>  - Use li instead of movptr
>  - VM leaf should use li
>  - Merge branch 'master' into jal-fixes
>  - Merge branch 'master' into jal-fixes
>  - Merge branch 'master' into jal-fixes
>  - Corrected method name
>  - ... and 2 more: https://git.openjdk.org/jdk/compare/b8d8e2ca...d53e9694

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 717:

> 715:     return x < (twoG - twoK) && x >= (-twoG - twoK);
> 716:   }
> 717: 

Can you add a code comment for this function to make it easier to understand? Maybe something like:
`Ensure that the auipc can reach the destination at x from anywhere within the code cache so that if it is relocated we know it will still reach.`

src/hotspot/cpu/riscv/macroAssembler_riscv.hpp line 738:

> 736:   typedef void (MacroAssembler::* load_insn_by_temp)(Register Rt, address dest, Register temp);
> 737: 
> 738:   void wrap_label(Register r, Label &L, Register t, load_insn_by_temp insn);

Better to remove the `load_insn_by_temp` declaration from file macroAssembler_riscv.hpp as it is not used anymore after this change.

`typedef void (MacroAssembler::* load_insn_by_temp)(Register Rt, address dest, Register temp);`

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

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