8263006: Add optimization for Max(*)Node and Min(*)Node #3513
Conversation
|
👋 Welcome back whuang! A progress list of the required criteria for merging this PR into |
|
/contributor add Wang Huang whuang@openjdk.org |
|
@Wanghuang-Huawei The following label will be automatically applied to this pull request:
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. |
|
@Wanghuang-Huawei |
|
@Wanghuang-Huawei |
Webrevs
|
|
@vnkozlov Can you do me a favor to review this optimization? Thank you very much. |
|
Do you have a real example in Java applications which benefit from this optimization? Optimization will not work for Integer because of I am not sure if this optimization will always work for float/double because of NaN values. You need to verify results for all edge cases. |
|
Thank you for your review.
Yes. We refined this optimization from our internal software experience. For instance, the model
Yes. Adding
I have tested that and showed in my comments. The test cases for NaN values and other special values are listed here import java.lang.Math;
public class Test {
public static void main(String[] args) throws Exception {
Test m = new Test();
m.test();
}
public void test() throws Exception {
double[] num = new double[9];
num[0] = 1; num[1] = 0; num[2] = -0;
num[3] = Double.POSITIVE_INFINITY;
num[4] = Double.NEGATIVE_INFINITY;
num[5] = Double.NaN;
num[6] = Double.MAX_VALUE;
num[7] = Double.MIN_VALUE;
num[8] = Double.MIN_NORMAL;
for(int i = 0; i < 9; i++) {
for(int j = 0; j < 9; j++) {
check(add_opt(num[i], num[j]), (num[i] + num[j]));
check(mul_opt(num[i], num[j]), (num[i] * num[j]));
check(max_opt(num[i], num[j]), Math.max(num[i], num[j]));
check(min_opt(num[i], num[j]), Math.min(num[i], num[j]));
}
}
}
public void check(double a, double b) {
if (a != b) {
System.out.println("false");
System.out.println(a);
System.out.println(b);
System.out.println();
}
}
public double add_opt(double a, double b) throws Exception {
return Math.max(a, b) + Math.min(a, b);
}
public double mul_opt(double a, double b) throws Exception {
return Math.max(a, b) * Math.min(a, b);
}
public double max_opt(double a, double b) throws Exception {
return Math.max(Math.max(a, b), Math.min(a, b));
}
public double min_opt(double a, double b) throws Exception {
return Math.min(Math.max(a, b), Math.min(a, b));
}
}The Should I add the other test cases for |
|
Thank you for answers. |
There was a problem hiding this comment.
Running MyBenchmark test listed in the summary on x86_64 Haswell XEON with this patch results in:
# Benchmark: org.sample.MyBenchmark.testMax
# Run progress: 25.00% complete, ETA 00:03:57
# Fork: 1 of 1 # Warmup Iteration 1: 112.714 us/op
# Warmup Iteration 2: # To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc: SuppressErrorAt=/register.hpp:152 #
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/home/mgkwill/src/git/jdk/src/hotspot/share/asm/register.hpp:152), pid=3450238, tid=3450253
# assert(a != b && a != c && a != d && a != e && b != c && b != d && b != e && c != d && c != e && d != e) failed: registers must be different: a=0x0000000000000001, b=0x0000000000000001, c=0x0000000000000003, d=0x0000000000000004, e=0x0000000000000005
#
# JRE version: OpenJDK Runtime Environment (17.0) (slowdebug build 17-internal+0-adhoc.mgkwill.jdk)
AND
# Benchmark: org.sample.MyBenchmark.testMin
# Run progress: 25.00% complete, ETA 00:03:57
# Fork: 1 of 1
# Warmup Iteration 1: 112.445 us/op
# Warmup Iteration 2: # To suppress the following error report, specify this argument
# after -XX: or in .hotspotrc: SuppressErrorAt=/register.hpp:152
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (/home/mgkwill/src/git/jdk/src/hotspot/share/asm/register.hpp:152), pid=3450444, tid=3450481
# assert(a != b && a != c && a != d && a != e && b != c && b != d && b != e && c != d && c != e && d != e) failed: registers must be different: a=0x0000000000000001, b=0x0000000000000001, c=0x0000000000000003, d=0x0000000000000004, e=0x0000000000000005
#
# JRE version: OpenJDK Runtime Environment (17.0) (slowdebug build 17-internal+0-adhoc.mgkwill.jdk)
Otherwise the results are below.
Baseline
Benchmark Mode Cnt Score Error Units
MyBenchmark.testAdd avgt 5 149.581 ? 13.626 us/op
MyBenchmark.testMax avgt 5 223.362 ? 195.007 us/op
MyBenchmark.testMin avgt 5 206.096 ? 16.840 us/op
MyBenchmark.testMul avgt 5 149.366 ? 9.558 us/op
Finished running test 'micro:MyBenchmark'
Benchmark Mode Cnt Score Error Units
MyBenchmark.testAdd avgt 5 107.874 ? 0.148 us/op
MyBenchmark.testMul avgt 5 107.881 ? 0.015 us/op
Finished running test 'micro:MyBenchmark'
I run this test case by |
Certainly. Tomorrow morning (my time) I will re-run with your command and test on a few other Xeons. I need to re-run to get crash logs (I'm jumping around between patches and cleaned already) but I will post those when I can recreate. To run I copied MyBenchmark.java from summary to test/micro/org/openjdk/bench/vm/compiler/MyBenchmark.java This worked on master, but on checkout of pr/3513 caused the errors above. |
Thank you very much. I will check this issue. |
I have tested on my |
I fix a bug in my code. I am not sure if it is the reason of your crash. However, you can pull the latest code and test again. Thank you. |
Your update fixed the error I was seeing. My apologies for not getting back to you yesterday, it turned out to be busier than I projected. Perf numbers on Haswell Intel Xeon CPU E5-1603 v3 @ 2.80GHz: |
|
@Wanghuang-Huawei 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: 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 475 new commits pushed to the
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 (@vnkozlov) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
@Wanghuang-Huawei |
Thank you for your review and approval! |
|
/sponsor |
openjdk
bot
added
integrated
|
@vnkozlov @Wanghuang-Huawei Since your change was applied there have been 498 commits pushed to the
Your commit was automatically rebased without conflicts. Pushed as commit 599d07c. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
I optimize
maxandminby using these identitiesTest case
The result is listed here (aarch64):
before:
after:
NaNINFINITYand-INFINITYand got same result (before/after)Progress
Issue
Reviewers
Contributors
<whuang@openjdk.org><wuyan34@huawei.com>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3513/head:pull/3513$ git checkout pull/3513Update a local copy of the PR:
$ git checkout pull/3513$ git pull https://git.openjdk.java.net/jdk pull/3513/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 3513View PR using the GUI difftool:
$ git pr show -t 3513Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3513.diff