New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
8273454: C2: Transform (-a)*(-b) into a*b #5403
Changes from 4 commits
4c1e90a
1ed1d7e
b0761b1
c55a327
be01f17
28d123f
8f7f241
71aa6ac
f9d7d61
3f3eeb0
57d1ecf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,88 @@ | ||
/* | ||
* Copyright (c) 2021, Red Hat, Inc. All rights reserved. | ||
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
* | ||
* This code is free software; you can redistribute it and/or modify it | ||
* under the terms of the GNU General Public License version 2 only, as | ||
* published by the Free Software Foundation. | ||
* | ||
* This code is distributed in the hope that it will be useful, but WITHOUT | ||
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
* version 2 for more details (a copy is included in the LICENSE file that | ||
* accompanied this code). | ||
* | ||
* You should have received a copy of the GNU General Public License version | ||
* 2 along with this work; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
* | ||
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
* or visit www.oracle.com if you need additional information or have any | ||
* questions. | ||
*/ | ||
|
||
/** | ||
* @test | ||
* @bug 8270366 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The bug number is incorrect. |
||
* @summary Test transformation (-a)*(-b) = a*b | ||
* | ||
* @run main/othervm -XX:-TieredCompilation -XX:-BackgroundCompilation -XX:-UseOnStackReplacement TestNegMultiply | ||
* | ||
*/ | ||
|
||
public class TestNegMultiply { | ||
private static final int[][] intParams = { | ||
{Integer.MAX_VALUE, Integer.MAX_VALUE}, | ||
{Integer.MIN_VALUE, Integer.MIN_VALUE}, | ||
{Integer.MAX_VALUE, Integer.MIN_VALUE}, | ||
{232, 34}, | ||
{-23, 445}, | ||
{-244, -84}, | ||
{233, -99} | ||
}; | ||
|
||
private static int intTest(int a, int b) { | ||
return (-a) * (-b); | ||
} | ||
|
||
private static void runIntTest() { | ||
for (int index = 0; index < intParams.length; index ++) { | ||
int result = intTest(intParams[index][0], intParams[index][1]); | ||
for (int i = 0; i < 20_000; i++) { | ||
if (result != intTest(intParams[index][0], intParams[index][1])) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After some warmup iterations, You should also prevent inlining of The test you added with JDK-8270366 has the same problem. |
||
throw new RuntimeException("incorrect result"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
private static final long[][] longParams = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to https://git.openjdk.java.net/jdk/pull/5266, I would prefer random values for better coverage. |
||
{Long.MAX_VALUE, Long.MAX_VALUE}, | ||
{Long.MIN_VALUE, Long.MIN_VALUE}, | ||
{Long.MAX_VALUE, Long.MIN_VALUE}, | ||
{232L, 34L}, | ||
{-23L, 445L}, | ||
{-244L, -84L}, | ||
{233L, -99L} | ||
}; | ||
|
||
private static long longTest(long a, long b) { | ||
return (-a) * (-b); | ||
} | ||
|
||
private static void runLongTest() { | ||
for (int index = 0; index < intParams.length; index ++) { | ||
long result = longTest(longParams[index][0], longParams[index][1]); | ||
for (int i = 0; i < 20_000; i++) { | ||
if (result != longTest(longParams[index][0], longParams[index][1])) { | ||
throw new RuntimeException("incorrect result"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
public static void main(String[] args) { | ||
runIntTest(); | ||
runLongTest(); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking if it's a good idea to move these code into MulNode, as they were actually much the same with MulLNode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder that too, so is the rest of MulINode/MulLNode::Ideal() code (and many other places). I am not sure how to workaround the different types, any suggestions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a dogfood, but it works. https://gist.github.com/theRealELiu/328d62157975b1f20e3626b3ef747eb4
Too much abstraction makes the code hard to read. One needs to check the concrete class to identify what the code exactly is, E.g. In my patch, add_id() may be TypeInt::ZERO or TypeLong::Zero, even TypeD::ZERO. So I'm not sure if it's a good idea. Is there any guidelines to this issue, try to abstract them or make the readability in the first place? @TobiHartmann
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I would also prefer to move the optimization into
MulNode::Ideal
. @theRealELiu's patch is good but can be further improved by modifying the node inputs instead of returning a new node (similar to the other optimizations inMulNode::Ideal
).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also,
Type::is_zero_type
can be used to detect 0 and instead of checking the opcodes,Node::is_Sub
should be used.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks, I will make changes accordingly.