Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Tue, 19 Oct 2021 20:34:55 GMT, Vamsi Parasa wrote: >> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. >> This change show 1.87X improvement on a micro benchmark. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > refactoring to remove code duplication by using a common routine for > UMulHiLNode and MulHiLNode Thank you for spotting the stale comment. It will removed in another related commit that will be pushed soon... - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Wed, 20 Oct 2021 22:14:33 GMT, Vladimir Kozlov wrote: > Tests passed. You can integrate changes. Thanks Vladimir! What are the next steps to integrate the change? - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Tue, 19 Oct 2021 20:34:55 GMT, Vamsi Parasa wrote: >> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. >> This change show 1.87X improvement on a micro benchmark. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > refactoring to remove code duplication by using a common routine for > UMulHiLNode and MulHiLNode Tests passed. You can integrate changes. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Tue, 19 Oct 2021 20:34:55 GMT, Vamsi Parasa wrote: >> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. >> This change show 1.87X improvement on a micro benchmark. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > refactoring to remove code duplication by using a common routine for > UMulHiLNode and MulHiLNode Looks good. And I submitted testing. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Tue, 19 Oct 2021 20:34:55 GMT, Vamsi Parasa wrote: >> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. >> This change show 1.87X improvement on a micro benchmark. > > Vamsi Parasa has updated the pull request incrementally with one additional > commit since the last revision: > > refactoring to remove code duplication by using a common routine for > UMulHiLNode and MulHiLNode Marked as reviewed by sviswanathan (Reviewer). The patch looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Fri, 15 Oct 2021 20:19:31 GMT, Vladimir Kozlov wrote: >>> How you verified correctness of results? I suggest to extend >>> `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover unsigned >>> method. >> >> Tests for unsignedMultiplyHigh were already added in >> test/jdk//java/lang/Math/MultiplicationTests.java (in July 2021 by Brian >> Burkhalter). Used that test to verify the correctness of the results. > >> > How you verified correctness of results? I suggest to extend >> > `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover unsigned >> > method. >> >> Tests for unsignedMultiplyHigh were already added in >> test/jdk//java/lang/Math/MultiplicationTests.java (in July 2021 by Brian >> Burkhalter). Used that test to verify the correctness of the results. > > Good. It seems I have old version of the test. > Did you run it with -Xcomp? How you verified that intrinsic is used? @vnkozlov if the patch looks ok to you, could you please run this through your testing? - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Fri, 15 Oct 2021 19:31:24 GMT, Vamsi Parasa wrote: >> src/hotspot/share/opto/mulnode.cpp line 468: >> >>> 466: } >>> 467: >>> 468: >>> //= >> >> MulHiLNode::Value() and UMulHiLNode::Value() seem to be identical. Perhaps >> some refactoring would be in order, maybe make a common shared routine. > > Sure, will do the refactoring to use a shared routine. Pushed the refactored code to use a common routine for MulHiLNode::Value() and UMulHiLNode::Value(). Please review... - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. > This change show 1.87X improvement on a micro benchmark. Vamsi Parasa has updated the pull request incrementally with one additional commit since the last revision: refactoring to remove code duplication by using a common routine for UMulHiLNode and MulHiLNode - Changes: - all: https://git.openjdk.java.net/jdk/pull/5933/files - new: https://git.openjdk.java.net/jdk/pull/5933/files/cb30b268..a10a9fbe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5933=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5933=00-01 Stats: 25 lines in 2 files changed: 12 ins; 12 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5933.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5933/head:pull/5933 PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh [v2]
On Fri, 15 Oct 2021 21:04:12 GMT, Vamsi Parasa wrote: > > > How you verified correctness of results? I suggest to extend > > > `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover > > > unsigned method. > > > > > > Tests for unsignedMultiplyHigh were already added in > > test/jdk//java/lang/Math/MultiplicationTests.java (in July 2021 by Brian > > Burkhalter). Used that test to verify the correctness of the results. > > Good. It seems I have old version of the test. Did you run it with -Xcomp? > How you verified that intrinsic is used? I have verified that the intrinsic is being used by looking at the x86 assembly code generated by using perfasm profiler. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Fri, 15 Oct 2021 20:19:31 GMT, Vladimir Kozlov wrote: > > > How you verified correctness of results? I suggest to extend > > > `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover > > > unsigned method. > > > > > > Tests for unsignedMultiplyHigh were already added in > > test/jdk//java/lang/Math/MultiplicationTests.java (in July 2021 by Brian > > Burkhalter). Used that test to verify the correctness of the results. > > Good. It seems I have old version of the test. Did you run it with -Xcomp? > How you verified that intrinsic is used? The tests were not run with -Xcomp. Looked at the generated x86 code using the disassembler (hsdis) and verified that that correct instruction (mul) instead of (imul, without the intrinsic) was being generated. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Fri, 15 Oct 2021 19:19:13 GMT, Vamsi Parasa wrote: > > How you verified correctness of results? I suggest to extend > > `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover unsigned > > method. > > Tests for unsignedMultiplyHigh were already added in > test/jdk//java/lang/Math/MultiplicationTests.java (in July 2021 by Brian > Burkhalter). Used that test to verify the correctness of the results. Good. It seems I have old version of the test. Did you run it with -Xcomp? How you verified that intrinsic is used? - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Fri, 15 Oct 2021 16:14:25 GMT, Andrew Haley wrote: >> Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. >> This change show 1.87X improvement on a micro benchmark. > > src/hotspot/share/opto/mulnode.cpp line 468: > >> 466: } >> 467: >> 468: >> //= > > MulHiLNode::Value() and UMulHiLNode::Value() seem to be identical. Perhaps > some refactoring would be in order, maybe make a common shared routine. Sure, will do the refactoring to use a shared routine. > test/micro/org/openjdk/bench/java/lang/MathBench.java line 547: > >> 545: return Math.unsignedMultiplyHigh(long747, long13); >> 546: } >> 547: > > As far as I can tell, the JMH overhead dominates when trying to measure the > latency of events in the nanosecond range. `unsignedMultiplyHigh` should have > a latency of maybe 1.5-2ns. Is that what you saw? Yes, the JMH overhead was dominating the measurement of latency. The latency observed for `unsignedMultiplyHigh` was 2.3ns with the intrinsic enabled. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Fri, 15 Oct 2021 18:45:41 GMT, Vladimir Kozlov wrote: > How you verified correctness of results? I suggest to extend > `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover unsigned > method. Tests for unsignedMultiplyHigh were already added in test/jdk//java/lang/Math/MultiplicationTests.java (in July 2021 by Brian Burkhalter). Used that test to verify the correctness of the results. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Wed, 13 Oct 2021 18:55:10 GMT, Vamsi Parasa wrote: > Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. > This change show 1.87X improvement on a micro benchmark. How you verified correctness of results? I suggest to extend `test/jdk//java/lang/Math/MultiplicationTests.java` test to cover unsigned method. - Changes requested by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Wed, 13 Oct 2021 18:55:10 GMT, Vamsi Parasa wrote: > Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. > This change show 1.87X improvement on a micro benchmark. test/micro/org/openjdk/bench/java/lang/MathBench.java line 547: > 545: return Math.unsignedMultiplyHigh(long747, long13); > 546: } > 547: As far as I can tell, the JMH overhead dominates when trying to measure the latency of events in the nanosecond range. `unsignedMultiplyHigh` should have a latency of maybe 1.5-2ns. Is that what you saw? - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Wed, 13 Oct 2021 18:55:10 GMT, Vamsi Parasa wrote: > Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. > This change show 1.87X improvement on a micro benchmark. src/hotspot/share/opto/mulnode.cpp line 468: > 466: } > 467: > 468: > //= MulHiLNode::Value() and UMulHiLNode::Value() seem to be identical. Perhaps some refactoring would be in order, maybe make a common shared routine. - PR: https://git.openjdk.java.net/jdk/pull/5933
Re: RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
On Wed, 13 Oct 2021 18:55:10 GMT, Vamsi Parasa wrote: > Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. > This change show 1.87X improvement on a micro benchmark. The `java.lang.Math` change looks good, of course. On the rest I cannot comment but it is good to see something being done here. - PR: https://git.openjdk.java.net/jdk/pull/5933
RFR: 8275167: x86 intrinsic for unsignedMultiplyHigh
Optimize the new Math.unsignedMultiplyHigh using the x86 mul instruction. This change show 1.87X improvement on a micro benchmark. - Commit messages: - fix typo in function name - 8275167: x86 intrinsic for unsignedMultiplyHigh Changes: https://git.openjdk.java.net/jdk/pull/5933/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5933=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275167 Stats: 59 lines in 11 files changed: 59 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5933.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5933/head:pull/5933 PR: https://git.openjdk.java.net/jdk/pull/5933
