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

[Feilong Jiang]

On Tue, 28 Mar 2023 05:59:49 GMT, Dingli Zhang <dzhang 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

Thanks for the refactoring! I have some comments:

src/hotspot/cpu/riscv/assembler_riscv.hpp line 2159:

> 2157: #define INSN(NAME, funct3, op)                                                               \
> 2158:   void NAME(int32_t offset) {                                                                \
> 2159:     assert(is_simm12(offset) && ((offset % 2) == 0), "just checking");                       \

Could you please improve the assert message? like `invalid imm` or something more specific.

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.

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

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

PR Review: https://git.openjdk.org/jdk/pull/13202#pullrequestreview-1360323052
PR Review Comment: https://git.openjdk.org/jdk/pull/13202#discussion_r1150125595
PR Review Comment: https://git.openjdk.org/jdk/pull/13202#discussion_r1150108861
PR Review Comment: https://git.openjdk.org/jdk/pull/13202#discussion_r1150105222