RFR: 8305008: RISC-V: Factor out immediate checking functions from assembler_riscv.inline.hpp

[Dingli Zhang]

On Tue, 28 Mar 2023 06:55:16 GMT, Feilong Jiang <fjiang at openjdk.org> wrote:

>> The call-point to 'is_imm_in_range', 'is_unsigned_imm_in_range' and 'is_offset_in_range' in assembler_riscv.inline.hpp could to be replaced with shared code function 'is_simm' and 'is_uimm'.
>> 
>> For 'is_imm_in_range', if align_bits = 0, then the call point can be changed to call 'is_simm'. If align_bits = 1, then an additional summation judgement is added to determine if it is a multiple of 2. For 'is_unsigned_imm_in_range', we can first determine if it is greater than 0, and then call 'is_uimm' further. 'is_offset_in_range' can be changed directly to call 'is_simm'. This patch also refactored for is_valid_riscv64_address and movptr.
>> 
>> This patch removed the original functions from assembler_riscv.inline.hpp and defined some fixed-width checking functions based on the number of immediate or offset bits in the spec[1]. At the same time, we also adjusted two includes to fix the compilation error after the modification.
>> 
>> This patch also renames the function 'operand_valid_for_add_immediate' to 'operand_valid_for_add_sub_immediate' to make it consistent with its usage.
>> 
>> Finally, this patch modified the return value in os::non_memory_address_word to avoid an imm64 value of -1 in MacroAssembler::movptr.
>> 
>> [1] https://github.com/riscv/riscv-isa-manual/releases/download/draft-20230131-c0b298a/riscv-spec.pdf
>> 
>> ### Testing:
>> 
>> On Unmatched with release build:
>> - [x] Tier1 tests
>> - [ ] Tier2 tests
>> - [ ] Tier3 tests
>
> src/hotspot/cpu/riscv/assembler_riscv.hpp line 2685:
> 
>> 2683:           c_addi16sp(imm);                                                                   \
>> 2684:           return;                                                                            \
>> 2685:         } else if (Rd->is_compressed_valid() && (imm & 0b11) == 0x0 && (imm >= 0) && is_uimm10(imm)) { \
> 
> Is it necessary to check if `imm >= 0`? `imm` will be cast to unsigned value in `is_uimmx` eventually.

Thank you for pointing this out!
I will also remove the `imm12 >= 0` that has the same case above.

> src/hotspot/cpu/riscv/assembler_riscv.hpp line 2832:
> 
>> 2830:   virtual void bang_stack_with_offset(int offset) { Unimplemented(); }
>> 2831: 
>> 2832:   static bool is_simm5(int64_t x);
> 
> I think it's better to check alignment inside of `is_immx`, rather than call site of `is_immx`, e.g.
> 
> 
> int bool Assembler::is_simm5(int64_t x, int align = 0) {
>     return (x & right_n_bits(align) == 0) && is_simm(x, 5 + align);
> }
> 
> 
> then we can drop some uncommon offset width checking like `is_simm13` and `is_simm21`.

Currently there are very few alignment checking cases in the intermediate checking functions and the only alignment checking is whether align is 1. Can we keep it outside for better readability?
Maybe we can keep `is_simm13` and `is_simm21` to make the format more uniform.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/13202#discussion_r1150314447
PR Review Comment: https://git.openjdk.org/jdk/pull/13202#discussion_r1150311619