8273021: C2: Improve Add and Xor ideal optimizations #5266
Conversation
|
|
|
@kelthuzadx 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. |
Webrevs
|
|
/contributor add yulei lei.yul@alibaba-inc.com |
|
@kelthuzadx |
src/hotspot/share/opto/addnode.cpp
Outdated
| if (phase->type(in1->in(2)) == TypeInt::MINUS_1) { | ||
| return new SubINode(phase->makecon(TypeInt::ZERO), in1->in(1)); | ||
| } | ||
| } else if (op2 == Op_AddI && phase->type(in1) == TypeInt::MINUS_1) { |
There was a problem hiding this comment.
Why do you need to check both inputs for constant -1? Shouldn't AddNode::Ideal canonicalize the inputs and ensure that constants are moved to the second input?
jdk/src/hotspot/share/opto/addnode.hpp
Lines 52 to 54 in 599d07c
There was a problem hiding this comment.
Indeed, commute already moves loads and constants into right .
Changed.
| * @bug 8273021 | ||
| * @summary C2: Improve Add and Xor ideal optimizations | ||
| * @library /test/lib | ||
| * @run main/othervm -XX:-Inline -XX:-TieredCompilation -XX:TieredStopAtLevel=4 -XX:CompileCommand=compileonly,compiler.c2.TestAddXorIdeal::* compiler.c2.TestAddXorIdeal |
There was a problem hiding this comment.
What about -XX:CompileCommand=dontinline,compiler.c2.TestAddXorIdeal::test* Instead of disabling all inlining and limiting compilation?
There was a problem hiding this comment.
Changed. Magic number have been substituted by random number.
| Asserts.assertTrue(test1(i + 5) == -(i + 5)); | ||
| Asserts.assertTrue(test2(i - 7) == -(i - 7)); | ||
| Asserts.assertTrue(test3(i + 100) == -(i + 100)); | ||
| Asserts.assertTrue(test4(i - 1024) == -(i - 1024)); |
There was a problem hiding this comment.
What about using random numbers for better coverage?
src/hotspot/share/opto/addnode.cpp
Outdated
| // convert into "x+ -c0" in SubXNode::Ideal. So ~(x-1) will eventually | ||
| // be -1^(x+(-1)). | ||
| if (op1 == Op_AddI && phase->type(in2) == TypeInt::MINUS_1) { | ||
| if (phase->type(in1->in(2)) == TypeInt::MINUS_1) { |
There was a problem hiding this comment.
These two conditions could be combined.
There was a problem hiding this comment.
Yes, I've combined these conditions.
…ditional checks; random num in test
| * @bug 8273021 | ||
| * @summary C2: Improve Add and Xor ideal optimizations | ||
| * @library /test/lib | ||
| * @run main/othervm -XX:-TieredCompilation -XX:TieredStopAtLevel=4 |
There was a problem hiding this comment.
TieredStopAtLevel has no effect if Tiered Compilation is turned off. You can remove it.
| /* | ||
| * @test | ||
| * @bug 8273021 | ||
| * @summary C2: Improve Add and Xor ideal optimizations |
There was a problem hiding this comment.
The test needs * @key randomness
| } | ||
|
|
||
| public static void main(String... args) { | ||
| Random random = new Random(); |
There was a problem hiding this comment.
You should use Utils.getRandomInstance() from import jdk.test.lib.Utils to ensure that the seed is printed for reproducibility. You can check other tests for an example.
There was a problem hiding this comment.
That does make sense. Changed.
| long n1 = 0; | ||
| for (int i = -5_000; i < 5_000; i++) { | ||
| n = random.nextInt(); | ||
| Asserts.assertTrue(test1(i + n) == -(i + n)); |
There was a problem hiding this comment.
Now that you are using random numbers, can't you simply check Asserts.assertTrue(test1(n) == -n)? And just loop for a fixed number of iterations.
| public static void main(String... args) { | ||
| Random random = new Random(); | ||
| int n = 0; | ||
| long n1 = 0; |
There was a problem hiding this comment.
Should be declared in the loop.
There was a problem hiding this comment.
Do you mean declared within loop body? I've changed but it looks like a perference problem.
There was a problem hiding this comment.
Yes, in Java, local variables should be declared as close as possible to the point they are first used at (see, for example, Google's Java Style Guide). The declaration does not affect performance.
Here's how I would write the loop to improve readability:
for (int j = 0; j < 50_000; j++) {
int i = random.nextInt();
long l = random.nextLong();
Asserts.assertTrue(test1(i) == -i);
Asserts.assertTrue(test2(i) == -i);
Asserts.assertTrue(test3(l) == -l);
...
Summary: No need to use negative initial value for loop induction variable (as it is not even used), increase number of iterations to ensure C2 compilation (you are running without -Xbatch), use same random numbers per loop iteration, use more intuitive variable names.
But these are just code style details, feel free to ignore.
There was a problem hiding this comment.
Thanks for patient. I agree with your comment that using 0 as initial value and increasing iterations. :)
I try to use a and b as variable names since long l looks like long 1
|
@kelthuzadx This change now passes all automated pre-integration checks. 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 203 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.
|
|
Thanks @TobiHartmann and @theRealELiu for reviews. |
|
/integrate |
|
Going to push as commit a73c06d.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@kelthuzadx Pushed as commit a73c06d. |
Greetings. This patch adds the following identical equations for Add and Xor node, respectively, which probably drives further optimizations.
Verified by generated opto assembly, maybe an IR verification test can be added later.
Progress
Issue
Reviewers
Contributors
<lei.yul@alibaba-inc.com>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5266/head:pull/5266$ git checkout pull/5266Update a local copy of the PR:
$ git checkout pull/5266$ git pull https://git.openjdk.java.net/jdk pull/5266/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5266View PR using the GUI difftool:
$ git pr show -t 5266Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5266.diff