JDK-8314056 Remove runtime platform check from frem/drem #15210

asgibbons · 2023-08-09T16:48:36Z

Remove platform check and move code to stubGenerator. This fix increases performance by ~4.5%.

UPDATE: Subsequent commits increase performance gain to ~2x for AVX2, with no significant change to AVX512.

Tested tier1.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8314056: Remove runtime platform check from frem/drem (Enhancement - P4)

Reviewers

Sandhya Viswanathan (@sviswa7 - Reviewer)
Jatin Bhateja (@jatin-bhateja - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15210/head:pull/15210
$ git checkout pull/15210

Update a local copy of the PR:
$ git checkout pull/15210
$ git pull https://git.openjdk.org/jdk.git pull/15210/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15210

View PR using the GUI difftool:
$ git pr show -t 15210

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15210.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-09T16:50:16Z

👋 Welcome back sgibbons! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-08-09T16:53:36Z

@asgibbons The following label will be automatically applied to this pull request:

hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-08-09T17:11:28Z

Webrevs

sviswa7 · 2023-08-23T22:09:32Z

src/hotspot/cpu/x86/stubGenerator_x86_64_fmod.cpp

-    __ stmxcsr(Address(rsp, 0));
-    __ movl(rax, Address(rsp, 0));
-    __ movl(rcx, rax);
-    __ orl(rcx, 0x7f80);
-    __ movl(Address(rsp, 0x04), rcx);


No need to do stmxcsr, we could define mxcsr_rz on similar lines as mxcsr_std as part of StubGenerator::create_control_words() and use those in fmod.

sviswa7 · 2023-08-23T22:10:37Z

src/hotspot/cpu/x86/stubGenerator_x86_64_fmod.cpp

+    __ movl(rcx, rax);
+    __ orl(rcx, 0x7f80);
+    __ movl(Address(rsp, 0x04), rcx);
+
    __ andl(rax, 0x6000);
    __ cmpl(rax, 0x2000);
    __ jcc(Assembler::equal, L_10c1);


Some of the jcc and jmp could be generated as short branches where feasible.
Also the stub and the loop entries could be aligned to 16 bytes for better perf.

sviswa7 · 2023-08-24T22:08:50Z

The changes look good to me now. Could you please also get the performance of DremFrem jmh micro with/without your PR for both UseAVX=2 and UseAVX=3?

asgibbons · 2023-08-27T23:05:33Z

The changes look good to me now. Could you please also get the performance of DremFrem jmh micro with/without your PR for both UseAVX=2 and UseAVX=3?

Results of DremFrem:

Before these changes:

java -XX:UseAVX=2 -jar ./build/linux-x86_64-server-release/images/test/micro/benchmarks.jar -f 1 DremFrem:

Benchmark                      Mode  Cnt  Score   Error  Units
DremFrem.calcDoubleJava        avgt    5  5.645 ± 0.003  ns/op
DremFrem.calcFloatJava         avgt    5  6.044 ± 0.005  ns/op
DremFrem.cornercaseDoubleJava  avgt    5  5.462 ± 0.033  ns/op
DremFrem.cornercaseFloatJava   avgt    5  5.494 ± 0.045  ns/op

java -XX:UseAVX=3 -jar ./build/linux-x86_64-server-release/images/test/micro/benchmarks.jar -f 1 DremFrem:

Benchmark                      Mode  Cnt  Score   Error  Units
DremFrem.calcDoubleJava        avgt    5  2.916 ± 0.006  ns/op
DremFrem.calcFloatJava         avgt    5  4.016 ± 0.001  ns/op
DremFrem.cornercaseDoubleJava  avgt    5  5.506 ± 0.009  ns/op
DremFrem.cornercaseFloatJava   avgt    5  5.506 ± 0.010  ns/op

=============================================
WITH these changes:

java -XX:UseAVX=2 -jar ./build/linux-x86_64-server-release/images/test/micro/benchmarks.jar -f 1 DremFrem:

Benchmark                      Mode  Cnt  Score   Error  Units
DremFrem.calcDoubleJava        avgt    5  2.838 ± 0.001  ns/op
DremFrem.calcFloatJava         avgt    5  3.554 ± 0.002  ns/op
DremFrem.cornercaseDoubleJava  avgt    5  5.511 ± 0.022  ns/op
DremFrem.cornercaseFloatJava   avgt    5  5.467 ± 0.014  ns/op

java -XX:UseAVX=3 -jar ./build/linux-x86_64-server-release/images/test/micro/benchmarks.jar -f 1 DremFrem:

Benchmark                      Mode  Cnt  Score    Error  Units
DremFrem.calcDoubleJava        avgt    5  2.598 ±  0.001  ns/op
DremFrem.calcFloatJava         avgt    5  3.229 ±  0.001  ns/op
DremFrem.cornercaseDoubleJava  avgt    5  5.508 ±  0.014  ns/op
DremFrem.cornercaseFloatJava   avgt    5  5.511 ±  0.005  ns/op

sviswa7

The PR looks good to me. Thanks for considering all the review comments.

openjdk · 2023-08-28T17:37:27Z

@asgibbons This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8314056: Remove runtime platform check from frem/drem

Reviewed-by: sviswanathan, jbhateja

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 283 new commits pushed to the master branch:

1c598c2: 8312428: PKCS11 tests fail with NSS 3.91
b2728cb: 8189685: need PerfMemory class update and a volatile_static_field support in VMStructs
e22762c: 8314932: G1: Fix -Wconversion warnings for simple cases inside g1 folder
762b652: 8314573: G1: Heap resizing at Remark does not take existing eden regions into account
a4e97aa: 8314753: Remove support for @beaninfo, @todo, @since.unbundled, and @note
5cc64cc: 8309463: IGV: Dynamic graph layout algorithm
1d02507: 8314476: TestJstatdPortAndServer.java failed with "java.rmi.NoSuchObjectException: no such object in table"
93188bd: 8314569: (fs) Improve normalization of UnixPath for input with trailing slashes
8b8182d: 8315087: G1: Use uint for G1 flags indicating percentage
3dc266c: 8315070: RISC-V: Clean up platform dependent inline headers
... and 273 more: https://git.openjdk.org/jdk/compare/4577147993c2f87e6ba298a664acad5decc968f0...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sviswa7, @jatin-bhateja) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

sviswa7 · 2023-08-28T17:37:49Z

/label hotspot-compiler

openjdk · 2023-08-28T17:45:18Z

@sviswa7
The hotspot-compiler label was successfully added.

jatin-bhateja

Changes looks good to me, you may also include the results of following benchmark.
./test/micro/org/openjdk/bench/vm/compiler/pea/Blender.java

jatin-bhateja · 2023-08-29T12:18:28Z

src/hotspot/cpu/x86/stubGenerator_x86_64_fmod.cpp

-    __ jcc(Assembler::aboveEqual, L_117f);
-    __ jmp(L_11af);
-    __ align32();
+    __ jccb(Assembler::aboveEqual, L_117f);


This is a bounded label jump, Assembler::jcc should automatically optimize it with short jump encoding, but its not a blocker.

asgibbons · 2023-08-29T17:38:20Z

/integrate

openjdk · 2023-08-29T17:40:09Z

@asgibbons
Your change (at version 31e6000) is now ready to be sponsored by a Committer.

asgibbons · 2023-08-29T17:50:07Z

@jddarcy FYI. If you'd like to review, it would be appreciated. Thanks.

jatin-bhateja · 2023-08-30T01:27:45Z

/sponsor

openjdk · 2023-08-30T01:28:33Z

Going to push as commit ce2a7ea.
Since your change was applied there have been 283 commits pushed to the master branch:

1c598c2: 8312428: PKCS11 tests fail with NSS 3.91
b2728cb: 8189685: need PerfMemory class update and a volatile_static_field support in VMStructs
e22762c: 8314932: G1: Fix -Wconversion warnings for simple cases inside g1 folder
762b652: 8314573: G1: Heap resizing at Remark does not take existing eden regions into account
a4e97aa: 8314753: Remove support for @beaninfo, @todo, @since.unbundled, and @note
5cc64cc: 8309463: IGV: Dynamic graph layout algorithm
1d02507: 8314476: TestJstatdPortAndServer.java failed with "java.rmi.NoSuchObjectException: no such object in table"
93188bd: 8314569: (fs) Improve normalization of UnixPath for input with trailing slashes
8b8182d: 8315087: G1: Use uint for G1 flags indicating percentage
3dc266c: 8315070: RISC-V: Clean up platform dependent inline headers
... and 273 more: https://git.openjdk.org/jdk/compare/4577147993c2f87e6ba298a664acad5decc968f0...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-08-30T01:28:45Z

@jatin-bhateja @asgibbons Pushed as commit ce2a7ea.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

asgibbons added 3 commits August 3, 2023 14:33

Accelerate fmod/dmod by reducing pathlength.

eb542f6

Windows changes

5d96280

Fix register sage; compare against ah.

0bae5e9

openjdk bot added the rfr Pull request is ready for review label Aug 9, 2023

openjdk bot added the hotspot hotspot-dev@openjdk.org label Aug 9, 2023

Move mxcsr code after denom > num test.

5b1d251

sviswa7 reviewed Aug 23, 2023

View reviewed changes

mxcsr fix; address review comments

31e6000

sviswa7 approved these changes Aug 28, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated hotspot-compiler hotspot-compiler-dev@openjdk.org labels Aug 28, 2023

jatin-bhateja reviewed Aug 29, 2023

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Aug 29, 2023

jatin-bhateja approved these changes Aug 30, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Aug 30, 2023

openjdk bot closed this Aug 30, 2023

openjdk bot removed the ready Pull request is ready to be integrated label Aug 30, 2023

openjdk bot removed rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Aug 30, 2023

asgibbons deleted the fmod-opt branch June 24, 2024 19:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JDK-8314056 Remove runtime platform check from frem/drem #15210

JDK-8314056 Remove runtime platform check from frem/drem #15210

asgibbons commented Aug 9, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

mlbridge bot commented Aug 9, 2023 •

edited

Loading

sviswa7 Aug 23, 2023

sviswa7 Aug 23, 2023

sviswa7 commented Aug 24, 2023

asgibbons commented Aug 27, 2023

sviswa7 left a comment

openjdk bot commented Aug 28, 2023 •

edited

Loading

sviswa7 commented Aug 28, 2023

openjdk bot commented Aug 28, 2023

jatin-bhateja left a comment

jatin-bhateja Aug 29, 2023 •

edited

Loading

asgibbons commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

asgibbons commented Aug 29, 2023

jatin-bhateja commented Aug 30, 2023

openjdk bot commented Aug 30, 2023

openjdk bot commented Aug 30, 2023

JDK-8314056 Remove runtime platform check from frem/drem #15210

JDK-8314056 Remove runtime platform check from frem/drem #15210

Conversation

asgibbons commented Aug 9, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Aug 9, 2023

openjdk bot commented Aug 9, 2023

mlbridge bot commented Aug 9, 2023 • edited Loading

Webrevs

sviswa7 Aug 23, 2023

Choose a reason for hiding this comment

sviswa7 Aug 23, 2023

Choose a reason for hiding this comment

sviswa7 commented Aug 24, 2023

asgibbons commented Aug 27, 2023

sviswa7 left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 28, 2023 • edited Loading

sviswa7 commented Aug 28, 2023

openjdk bot commented Aug 28, 2023

jatin-bhateja left a comment

Choose a reason for hiding this comment

jatin-bhateja Aug 29, 2023 • edited Loading

Choose a reason for hiding this comment

asgibbons commented Aug 29, 2023

openjdk bot commented Aug 29, 2023

asgibbons commented Aug 29, 2023

jatin-bhateja commented Aug 30, 2023

openjdk bot commented Aug 30, 2023

openjdk bot commented Aug 30, 2023

asgibbons commented Aug 9, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 9, 2023 •

edited

Loading

openjdk bot commented Aug 28, 2023 •

edited

Loading

jatin-bhateja Aug 29, 2023 •

edited

Loading