RFR: 8319705: RISC-V: signumF/D intrinsics fails compiler/intrinsics/math/TestSignumIntrinsic.java [v2]

[Fei Yang]

On Wed, 8 Nov 2023 12:36:16 GMT, Hamlin Li <mli at openjdk.org> wrote:

>> Hi,
>> Can you review the patch to fix the bug introduced by [JDK-8317971](https://bugs.openjdk.org/browse/JDK-8317971) which implemented signumF/D intrinsics.
>> But seems the intrinsics failed since day 1 (I checkouted the commit to verify it.)
>> Thanks
>> 
>> ## Test
>> hotspot tests:
>> 
>> test/hotspot/jtreg/compiler/intrinsics/math/TestSignumIntrinsic.java
>> test/hotspot/jtreg/compiler/c2/PolynomialRoot.java
>> 
>> 
>> other jdk tests contains Math.signum(...):
>> 
>> test/jdk/java/math/BigInteger/BigIntegerTest.java 
>> test/jdk/java/math/BigInteger/largeMemory/DivisionOverflow.java
>> test/jdk/java/math/BigInteger/SerializationTests.java
>> test/jdk/java/math/BigInteger/ModInvTime.java
>> test/jdk/java/math/BigDecimal/SquareRootTests.java
>> test/jdk/java/math/BigDecimal/DivideTests.java
>> test/jdk/java/lang/Math/IeeeRecommendedTests.java
>> test/jdk/java/lang/Math/IeeeRemainderTests.java
>
> Hamlin Li has updated the pull request incrementally with one additional commit since the last revision:
> 
>   remove rFlagsReg

Changes requested by fyang (Reviewer).

src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1661:

> 1659: // one - gives us a floating-point 1.0 (got from matching rule)
> 1660: // bool is_double - specifies single or double precision operations will be used.
> 1661: void C2_MacroAssembler::signum_fp(FloatRegister dst, FloatRegister one, Register tmp, bool is_double) {

I think we could still make use of scratch register `t0` in this function. That will save us one GP register.

src/hotspot/cpu/riscv/riscv.ad line 7604:

> 7602: %}
> 7603: 
> 7604: instruct signumD_reg(fRegD dst, immD zero, fRegD one, iRegLNoSp tmp) %{

No need to reserve `tmp` here. See my previous comment.

src/hotspot/cpu/riscv/riscv.ad line 7615:

> 7613: %}
> 7614: 
> 7615: instruct signumF_reg(fRegF dst, immF zero, fRegF one, iRegLNoSp tmp) %{

No need to reserve `tmp` here. See my previous comment.

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

PR Review: https://git.openjdk.org/jdk/pull/16557#pullrequestreview-1721561669
PR Review Comment: https://git.openjdk.org/jdk/pull/16557#discussion_r1387392303
PR Review Comment: https://git.openjdk.org/jdk/pull/16557#discussion_r1387393032
PR Review Comment: https://git.openjdk.org/jdk/pull/16557#discussion_r1387393160