Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Thu, 23 May 2024 10:12:17 GMT, Bhavana Kilambi wrote: >> Yudi Zheng has updated the pull request incrementally with one additional >> commit since the last revision: >> >> address comments. > > src/hotspot/share/opto/library_call.cpp line 5925: > >> 5923: // Set the original stack and the reexecute bit for the interpreter >> to reexecute >> 5924: // the bytecode that invokes BigInteger.multiplyToLen() if >> deoptimization happens >> 5925: // on the return from z array allocation in runtime. > > Since we are not allocating z array during runtime anymore, do we still need > these comments? Thanks for pointing it out! Removed. > src/java.base/share/classes/java/math/BigInteger.java line 1836: > >> 1834: >> 1835: if (z == null || z.length < (xlen + ylen)) >> 1836: z = new int[xlen + ylen]; > > Style: only 4 spaces indentation Done - PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1613608426 PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1613608653
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. Hi, RISC-V part of change seems fine. "java/math/BigInteger" test result is clean on linux-riscv64 platform. Thanks for the ping. - Marked as reviewed by fyang (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18226#pullrequestreview-2076019194
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. This is good. Please, merge latest mainline and rerun mach5 testing. - Marked as reviewed by kvn (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18226#pullrequestreview-2075281290
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. Tested tier1 on aarch64 and no failures. Also no regressions (or even gain) on aarch64 with the BigInteger testcase you mentioned. I think copyright year has not been updated for some of the files but I guess that's up to you. - PR Comment: https://git.openjdk.org/jdk/pull/18226#issuecomment-2127335211
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. src/java.base/share/classes/java/math/BigInteger.java line 1836: > 1834: > 1835: if (z == null || z.length < (xlen + ylen)) > 1836: z = new int[xlen + ylen]; Style: only 4 spaces indentation - PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1611422191
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. src/hotspot/share/opto/library_call.cpp line 5925: > 5923: // Set the original stack and the reexecute bit for the interpreter > to reexecute > 5924: // the bytecode that invokes BigInteger.multiplyToLen() if > deoptimization happens > 5925: // on the return from z array allocation in runtime. Since we are not allocating z array during runtime anymore, do we still need these comments? - PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1611403873
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. I have looked into s390x code and do not see any issue there. Also run test for BigInteger separately and a round of tier1 test as well. Result is clean there as well. - Marked as reviewed by amitkumar (Committer). PR Review: https://git.openjdk.org/jdk/pull/18226#pullrequestreview-2073070871
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. PPC64 part and shared code looks correct. "java/math/BigInteger" tests have passed on PPC64. I didn't review the other platforms. - Marked as reviewed by mdoerr (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/18226#pullrequestreview-2072434441
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:28:41 GMT, Yudi Zheng wrote: >> src/hotspot/cpu/aarch64/stubGenerator_aarch64.cpp line 4693: >> >>> 4691: const Register xlen = r1; >>> 4692: const Register z = r2; >>> 4693: const Register zlen = r3; >> >> LibraryCallKit::inline_squareToLen() is still computing zlen and passing it >> as the 4th arg, even though the value is unused. > > ppc x86 are not using `multiply_to_len` for `generate_squareToLen`. I think > we still need to pass zlen for these platforms. OK. - PR Review Comment: https://git.openjdk.org/jdk/pull/18226#discussion_r1610580527
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
On Wed, 22 May 2024 14:47:43 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 comments. @theRealAph @TheRealMDoerr @RealFYang @offamitkumar could you please help review the aarch64/ppc/riscv/s390 changes? - PR Comment: https://git.openjdk.org/jdk/pull/18226#issuecomment-2125073511
Re: RFR: 8327964: Simplify BigInteger.implMultiplyToLen intrinsic [v6]
> 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 comments. - Changes: - all: https://git.openjdk.org/jdk/pull/18226/files - new: https://git.openjdk.org/jdk/pull/18226/files/72ba58ce..7c6023f8 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18226=05 - incr: https://webrevs.openjdk.org/?repo=jdk=18226=04-05 Stats: 24 lines in 2 files changed: 0 ins; 0 del; 24 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
