On Mon, 16 Oct 2023 21:14:39 GMT, Ilya Gavrilin <igavri...@openjdk.org> wrote:
>> Hi all, please review this changes into risc-v floating point copysign and
>> signum intrinsics.
>> CopySign - returns first argument with the sign of second. On risc-v we have
>> `fsgnj.x` instruction, which can implement this intrinsic.
>> Signum - returns input value if it is +/- 0.0 or NaN, otherwise 1.0 with the
>> sign of input value returned. On risc-v we can use `fclass.x` to specify
>> type of input value and return appropriate value.
>>
>> Tests:
>> Performance tests on t-head board:
>> With intrinsics:
>>
>> Benchmark (seed) Mode Cnt Score Error Units
>> MathBench.copySignDouble 0 thrpt 8 34156.580 ± 76.272 ops/ms
>> MathBench.copySignFloat 0 thrpt 8 34181.731 ± 38.182 ops/ms
>> MathBench.signumDouble 0 thrpt 8 31977.258 ± 1122.327 ops/ms
>> MathBench.signumFloat 0 thrpt 8 31836.852 ± 56.013 ops/ms
>>
>> Intrinsics turned off (`-XX:+UnlockDiagnosticVMOptions
>> -XX:-UseCopySignIntrinsic -XX:-UseSignumIntrinsic`):
>>
>> Benchmark (seed) Mode Cnt Score Error Units
>> MathBench.copySignDouble 0 thrpt 8 31000.996 ± 943.094 ops/ms
>> MathBench.copySignFloat 0 thrpt 8 30678.016 ± 28.087 ops/ms
>> MathBench.signumDouble 0 thrpt 8 25435.010 ± 2047.085 ops/ms
>> MathBench.signumFloat 0 thrpt 8 25257.058 ± 79.175 ops/ms
>>
>> Regression tests: tier1, hotspot:tier2 on risc-v board.
>>
>> Also, changed name of one micro test: before we had: `sigNumDouble` and
>> `signumFloat` tests, they does not matches to `signum` or `sigNum`. Now we
>> have similar part: `signum`.
>> Performance tests has been changed a bit, to check intrinsics result better,
>> diff to modify tests:
>>
>> diff --git a/test/micro/org/openjdk/bench/java/lang/MathBench.java
>> b/test/micro/org/openjdk/bench/java/lang/MathBench.java
>> index 6cd1353907e..0bee25366bf 100644
>> --- a/test/micro/org/openjdk/bench/java/lang/MathBench.java
>> +++ b/test/micro/org/openjdk/bench/java/lang/MathBench.java
>> @@ -143,12 +143,12 @@ public double ceilDouble() {
>>
>> @Benchmark
>> public double copySignDouble() {
>> - return Math.copySign(double81, doubleNegative12);
>> + return Math.copySign(double81, doubleNegative12) +
>> Math.copySign(double81, double2) + Math.copySign(double4Dot1,
>> doubleNegative12);
>> }
>>
>> @Benchmark
>> public float copySignFloat() {
>> - return Math.copySign(floatNegative99, float1);
>> + return ...
>
> Ilya Gavrilin has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix some registers usages and typos
Changes requested by fyang (Reviewer).
src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1659:
> 1657: // on input we have NaN or +/-0.0 value we should return it,
> 1658: // otherwise return +/- 1.0 using sign of input.
> 1659: // tmp1 - alias for t0 register,
Maybe remove this line (L1659) of the comment?
src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1663:
> 1661: // bool is_double - specififes single or double precision operations
> will be used.
> 1662: void C2_MacroAssembler::signum_fp(FloatRegister dst, FloatRegister src,
> FloatRegister one, bool is_double) {
> 1663: assert_different_registers(dst, src, one);
Any reason to keep the assertion?
src/hotspot/cpu/riscv/c2_MacroAssembler_riscv.cpp line 1682:
> 1680: // use floating-point 1.0 with a sign of input
> 1681: is_double ? fsgnj_d(dst, one, src)
> 1682: : fsgnj_s(dst, one, src);
What if the `src` argument contains zero? Math.signum(float/double) is supposed
to return zero if the argument is zero [1].
[1]
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Math.java#L2602
src/hotspot/cpu/riscv/riscv.ad line 7537:
> 7535: instruct signumD_reg(fRegD dst, fRegD src, immD zero, fRegD one) %{
> 7536: match(Set dst (SignumD src (Binary zero one)));
> 7537: effect(TEMP_DEF dst, USE src, USE one);
Any reason to keep this effect?
src/hotspot/cpu/riscv/riscv.ad line 7548:
> 7546: instruct signumF_reg(fRegF dst, fRegF src, immF zero, fRegF one) %{
> 7547: match(Set dst (SignumF src (Binary zero one)));
> 7548: effect(TEMP_DEF dst, USE src, USE one);
Any reason to keep this effect?
-------------
PR Review: https://git.openjdk.org/jdk/pull/16186#pullrequestreview-1681791129
PR Review Comment: https://git.openjdk.org/jdk/pull/16186#discussion_r1361790804
PR Review Comment: https://git.openjdk.org/jdk/pull/16186#discussion_r1361788992
PR Review Comment: https://git.openjdk.org/jdk/pull/16186#discussion_r1361782859
PR Review Comment: https://git.openjdk.org/jdk/pull/16186#discussion_r1361793207
PR Review Comment: https://git.openjdk.org/jdk/pull/16186#discussion_r1361793356
