RFR: 8321001: RISC-V: C2 SignumVF

[Fei Yang]

On Fri, 1 Dec 2023 15:24:35 GMT, Hamlin Li <mli at openjdk.org> wrote:

> Hi,
> Can you review the patch to add intrinisc SignumVF/SignumVD on riscv?
> Thanks
> 
> ## Test
> test/hotspot/jtreg/compiler/intrinsics/
> test/hotspot/jtreg/compiler/vectorapi/
> and tests found via:
> grep -nr test/hotspot/jtreg/ -we Math.signum
> and test found via:
> grep -nr test/jdk/ -we Math.signum

Hi, Did you check the C2 JIT code? I am wondering whether the newly-added code is covered well by the tests performed.

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

> 1590:   INSN(vfsgnj_vf, 0b1010111, 0b101, 0b001000);
> 1591:   INSN(vfsgnjx_vf, 0b1010111, 0b101, 0b001010);
> 1592:   INSN(vfsgnjn_vf, 0b1010111, 0b101, 0b001001);

Not used anywhere?

src/hotspot/cpu/riscv/riscv_v.ad line 3670:

> 3668:   match(Set dst (SignumVF dst (Binary zero one)));
> 3669:   match(Set dst (SignumVD dst (Binary zero one)));
> 3670:   effect(TEMP_DEF dst);

v0 is clobbered in `C2_MacroAssembler::signum_fp_v`. Shouldn't we add a `TEMP v0` to the effect?

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

Changes requested by fyang (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/16925#pullrequestreview-1761692106
PR Review Comment: https://git.openjdk.org/jdk/pull/16925#discussion_r1413430803
PR Review Comment: https://git.openjdk.org/jdk/pull/16925#discussion_r1413430160