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

[Robbin Ehn]

On Wed, 22 May 2024 06:42:56 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> 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
>
> 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`

We already have this defined in nativeinst, I used that enum.

> 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/

Fixed

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

Fixed

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

Fixed

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

Fixed

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

Fixed

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609535038
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609535247
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609533925
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609533631
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609534062
PR Review Comment: https://git.openjdk.org/jdk/pull/19246#discussion_r1609533778