RFR: 8332265: RISC-V: Materialize pointers faster by using a temp register [v3]

[Fei Yang]

On Tue, 21 May 2024 08:56:19 GMT, Robbin Ehn <rehn at openjdk.org> wrote:

>> Hi, please consider!
>> 
>> Materializing a 48-bit pointer, using an additional register, we can do with:
>> lui + lui + slli + add + addi
>> This 15% faster both on VF2 and in CPU models, compared to movptr().
>> 
>> As we often materialize during calls there is free registers.
>> 
>> I have choose just a few spot to use it, many more can use.
>> E.g. la() with tmp register can use li48 instead of movptr.
>> 
>> Running tests now (so far so good), as if I screwed up IC calls it should be seen fast.
>> And benchmarks when hardware is free.
>
> 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 five additional commits since the last revision:
> 
>  - Merge branch 'master' into 8332265
>  - Small review update
>  - li48 -> movptr
>  - Merge branch 'master' into 8332265
>  - li48

Hi, Nice work! I only have several minor comments.

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1423:

> 1421:   Assembler::patch(branch + 12, 31, 20, (lower >> 6) & 0x7ff);                  // Addi.            target[16: 6] ==> branch[31:20]
> 1422:   Assembler::patch(branch + 20, 31, 20, lower & 0x3f);                          // Addi/Jalr/Load.  target[ 5: 0] ==> branch[31:20]
> 1423:   return MOVPTR_INSTRUCTIONS_NUM * NativeInstruction::instruction_size;

Maybe rename `MOVPTR_INSTRUCTIONS_NUM` as `MOVPTR1_INSTRUCTIONS_NUM`? (And `MOVPTR2_INSTRUCTIONS_NUM` for `patch_addr_in_movptr2` at the same time)
Or simply remove `MOVPTR_INSTRUCTIONS_NUM`:
`return 6 * NativeInstruction::instruction_size;  // lui + addi + slli + addi + slli + addi/jalr/load`

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1565:

> 1563:   } else if (NativeInstruction::is_pc_relative_at(instruction_address)) {          // auipc, addi/jalr/load
> 1564:     return patch_offset_in_pc_relative(instruction_address, offset);
> 1565:   } else if (NativeInstruction::is_movptr1_at(instruction_address)) {              // movptr

Code comment: s/movptr/movptr1/

src/hotspot/cpu/riscv/macroAssembler_riscv.cpp line 1598:

> 1596:     offset = get_offset_of_pc_relative(insn_addr);
> 1597:   } else if (NativeInstruction::is_movptr1_at(insn_addr)) {          // movptr
> 1598:     return get_target_of_movptr(insn_addr);

Maybe rename `get_target_of_movptr` as `get_target_of_movptr1` so that it will be consistent in naming?

src/hotspot/cpu/riscv/nativeInst_riscv.hpp line 381:

> 379:   enum RISCV_specific_constants {
> 380:     movptr1_instruction_size            =    6 * NativeInstruction::instruction_size, // lui, addi, slli, addi, slli, addi.  See movptr().
> 381:     movptr2_instruction_size            =    5 * NativeInstruction::instruction_size, // lui, lui, slli, add, addi.  See movptr2_imp().

Code comment needs update. No `movptr2_imp`?

src/hotspot/cpu/riscv/nativeInst_riscv.hpp line 403:

> 401:       // Assume: auipc, ld
> 402:       return addr_at(load_pc_relative_instruction_size);
> 403:     } else if (is_movptr2_at(instruction_address())) {

Move this after the `if (is_movptr1_at(instruction_address())) {` check to group the two together as always?

src/hotspot/cpu/riscv/riscv.ad line 1290:

> 1288:   // skip the movptr2 in MacroAssembler::ic_call():
> 1289:   // lui + addi + slli + addi + slli + addi
> 1290:   // Though movptr() has already 4-byte aligned with or without RVC,

Code comment here needs update too.

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

Changes requested by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/19246#pullrequestreview-2069949779
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609375185
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609378500
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609190379
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609151043
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609241656
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609157823