Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v2]

[Dean Long]

On Tue, 19 Mar 2024 19:06:36 GMT, Yudi Zheng  wrote:

>> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
>> candidate to its caller simplifies the intrinsic implementation in JIT 
>> compiler.
>
> Yudi Zheng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address comment.

src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4670:

> 4668: const Register tmp6  = r15;
> 4669: const Register tmp7  = r16;
> 4670: const Register tmp8  = r17;

It looks like tmp8 is never used.  The call to multiply_to_len() below needs to 
be adjusted.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1530986811

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v2]

[Dean Long]

On Tue, 19 Mar 2024 19:06:36 GMT, Yudi Zheng  wrote:

>> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
>> candidate to its caller simplifies the intrinsic implementation in JIT 
>> compiler.
>
> Yudi Zheng has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   address comment.

src/hotspot/cpu/aarch64/macroAssembler_aarch64.cpp line 3559:

> 3557:  Register tmp5, Register tmp6, 
> Register product_hi, Register tmp8) {
> 3558: 
> 3559:   assert_different_registers(x, xlen, y, ylen, z, tmp1, tmp2, tmp3, 
> tmp4, tmp5, tmp6, tmp8);

Suggestion:

  assert_different_registers(x, xlen, y, ylen, z, tmp1, tmp2, tmp3, tmp4, tmp5, 
tmp6, tmp8, product_hi);

-

PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1530980566

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v2]

[Yudi Zheng]

> Moving array construction within BigInteger.implMultiplyToLen intrinsic 
> candidate to its caller simplifies the intrinsic implementation in JIT 
> compiler.

Yudi Zheng has updated the pull request incrementally with one additional 
commit since the last revision:

  address comment.

-

Changes:
  - all: https://git.openjdk.org/jdk/pull/18226/files
  - new: https://git.openjdk.org/jdk/pull/18226/files/37232a5f..bfc323b7

Webrevs:
 - full: https://webrevs.openjdk.org/?repo=jdk=18226=01
 - incr: https://webrevs.openjdk.org/?repo=jdk=18226=00-01

  Stats: 70 lines in 11 files changed: 8 ins; 28 del; 34 mod
  Patch: https://git.openjdk.org/jdk/pull/18226.diff
  Fetch: git fetch https://git.openjdk.org/jdk.git pull/18226/head:pull/18226

PR: https://git.openjdk.org/jdk/pull/18226

Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v2]

[Yudi Zheng]

On Mon, 18 Mar 2024 16:55:28 GMT, Damon Fenacci  wrote:

> Quite a simplification! Have you checked if there are any performance 
> differences?

Ran 
https://github.com/oracle/graal/blob/master/compiler/src/org.graalvm.micro.benchmarks/src/micro/benchmarks/BigIntegerBenchmark.java
The results are

$ before change
Benchmark Mode  CntScore   Error  Units
BigIntegerBenchmark.bigIntMontgomeryMul  thrpt5  122.488 ± 0.130  ops/s
BigIntegerBenchmark.bigIntMontgomerySqr  thrpt5   76.023 ± 0.106  ops/s
BigIntegerBenchmark.bigIntMulthrpt5  330.130 ± 0.349  ops/s
BigIntegerBenchmark.bigIntMulAdd thrpt5  455.590 ± 0.663  ops/s

$ after change
Benchmark Mode  CntScore   Error  Units
BigIntegerBenchmark.bigIntMontgomeryMul  thrpt5  124.407 ± 0.045  ops/s
BigIntegerBenchmark.bigIntMontgomerySqr  thrpt5   76.036 ± 0.232  ops/s
BigIntegerBenchmark.bigIntMulthrpt5  329.836 ± 0.953  ops/s
BigIntegerBenchmark.bigIntMulAdd thrpt5  456.485 ± 0.766  ops/s

-

PR Comment: https://git.openjdk.org/jdk/pull/18226#issuecomment-2007922439