RFR: 8346478: RISC-V: Refactor add/sub assembler routines [v5]

[Feilong Jiang]

On Sat, 21 Dec 2024 06:24:04 GMT, Fei Yang <fyang at openjdk.org> wrote:

>> Hi, please consider this cleanup change.
>> 
>> Currently, we have mixed use of `addi` and `add(int64_t)`/`sub(int64_t)`. The former adds a 12-bit immediate while the latter
>> does not have a constraint on the immediate range. We should use `addi` when possible, which would help save one runtime check about the immediate range and avoid the use of one tmp register by the latter as well.
>> 
>> In order to make the code more readable, this also introduces helper routines `subi`/`subiw` and adapts callsites of `addi`/`addiw` with negative immediates.
>> 
>> 
>> 
>>  <Design of the RISC-V Instruction Set Architecture>:
>> 
>>  There is no SUBI instruction, because ADDI with a negative immediate is almost equivalent. The one
>>  exception arises from the asymmetry of the twos complement representation: SUBI with an immediate of
>>  211 would add 211 to a register, which ADDI is incapable of.
>> 
>> 
>> Testing on Premier-P550 SBC running Ubuntu-24.04:
>> - [x] : tier1-3 and gtest:all (release)
>> - [x] : hotspot:tier1 (fastdebug)
>
> Fei Yang has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Revert unnecessary change

Looks good! I have some minor comments.

src/hotspot/cpu/riscv/interp_masm_riscv.cpp line 1755:

> 1753: 
> 1754:       int to_add = in_bytes(TypeStackSlotEntries::per_arg_size());
> 1755:       add(off_to_args, off_to_args, to_add);

Do we need to use `add` here? I think `addi` is okay.

src/hotspot/cpu/riscv/interp_masm_riscv.cpp line 1852:

> 1850:     add(mdp, mdp, tmp1);
> 1851:     ld(tmp1, Address(mdp, ArrayData::array_len_offset()));
> 1852:     sub(tmp1, tmp1, TypeStackSlotEntries::per_arg_count());

`per_arg_count()` returns `2`, maybe we could use `subi`

src/hotspot/cpu/riscv/interp_masm_riscv.cpp line 1878:

> 1876: 
> 1877:     // go to next parameter
> 1878:     sub(tmp1, tmp1, TypeStackSlotEntries::per_arg_count());

We can use `subi` too.

src/hotspot/cpu/riscv/templateTable_riscv.cpp line 200:

> 198:     // if a breakpoint is present we can't rewrite the stream directly
> 199:     __ load_unsigned_byte(temp_reg, at_bcp(0));
> 200:     __ sub(temp_reg, temp_reg, Bytecodes::_breakpoint); // temp_reg is temporary register.

Should we use `subi` here?

src/hotspot/cpu/riscv/templateTable_riscv.cpp line 212:

> 210:   __ load_unsigned_byte(temp_reg, at_bcp(0));
> 211:   __ beq(temp_reg, bc_reg, L_okay);
> 212:   __ sub(temp_reg, temp_reg, (int)Bytecodes::java_code(bc));

ditto

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

PR Review: https://git.openjdk.org/jdk/pull/22804#pullrequestreview-2519615588
PR Review Comment: https://git.openjdk.org/jdk/pull/22804#discussion_r1894911494
PR Review Comment: https://git.openjdk.org/jdk/pull/22804#discussion_r1894912306
PR Review Comment: https://git.openjdk.org/jdk/pull/22804#discussion_r1894912476
PR Review Comment: https://git.openjdk.org/jdk/pull/22804#discussion_r1894915970
PR Review Comment: https://git.openjdk.org/jdk/pull/22804#discussion_r1894916145