Skip to content
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

Closed
wants to merge 11 commits into from
@@ -63,9 +63,19 @@ Node *MulNode::Ideal(PhaseGVN *phase, bool can_reshape) {
const Type *t2 = phase->type( in(2) );
Node *progress = NULL; // Progress flag

// convert "max(a,b) * min(a,b)" into "a*b".
// convert "(-a)*(-b)" into "a*b"
Node *in1 = in(1);
Node *in2 = in(2);
if (in1->is_Sub() && in2->is_Sub()) {
Node* n11 = in1->in(1);
Copy link
Member

@TobiHartmann TobiHartmann Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with below code, I would name the local in11 or simply use phase->type(in1->in(1)) because it's the only user.

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Node* n21 = in2->in(1);
if (phase->type(n11)->is_zero_type() &&
phase->type(n21)->is_zero_type()) {
return make(in1->in(2), in2->in(2));
Copy link
Member

@TobiHartmann TobiHartmann Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need to create a new node? Can't you simply update the inputs like the code below does?

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed your early comment. Fixed.

}
}

// convert "max(a,b) * min(a,b)" into "a*b".
if ((in(1)->Opcode() == max_opcode() && in(2)->Opcode() == min_opcode())
|| (in(1)->Opcode() == min_opcode() && in(2)->Opcode() == max_opcode())) {
Node *in11 = in(1)->in(1);
@@ -80,6 +80,8 @@ class MulNode : public Node {

// Supplied function to return the multiplicative opcode
virtual int min_opcode() const = 0;

virtual MulNode* make(Node* in1, Node* in2) const = 0;
};

//------------------------------MulINode---------------------------------------
@@ -98,6 +100,7 @@ class MulINode : public MulNode {
int min_opcode() const { return Op_MinI; }
const Type *bottom_type() const { return TypeInt::INT; }
virtual uint ideal_reg() const { return Op_RegI; }
virtual MulNode* make(Node* in1, Node* in2) const { return new MulINode(in1, in2); }
};

//------------------------------MulLNode---------------------------------------
@@ -116,6 +119,7 @@ class MulLNode : public MulNode {
int min_opcode() const { return Op_MinL; }
const Type *bottom_type() const { return TypeLong::LONG; }
virtual uint ideal_reg() const { return Op_RegL; }
virtual MulNode* make(Node* in1, Node* in2) const { return new MulLNode(in1, in2); }
};


@@ -134,6 +138,7 @@ class MulFNode : public MulNode {
int min_opcode() const { return Op_MinF; }
const Type *bottom_type() const { return Type::FLOAT; }
virtual uint ideal_reg() const { return Op_RegF; }
virtual MulNode* make(Node* in1, Node* in2) const { return new MulFNode(in1, in2); }
};

//------------------------------MulDNode---------------------------------------
@@ -151,6 +156,7 @@ class MulDNode : public MulNode {
int min_opcode() const { return Op_MinD; }
const Type *bottom_type() const { return Type::DOUBLE; }
virtual uint ideal_reg() const { return Op_RegD; }
virtual MulNode* make(Node* in1, Node* in2) const { return new MulDNode(in1, in2); }
};

//-------------------------------MulHiLNode------------------------------------
@@ -0,0 +1,129 @@
/*
* 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 8273454
Copy link
Member

@TobiHartmann TobiHartmann Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test needs * @key randomness

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* @summary Test transformation (-a)*(-b) = a*b
*
* @run main/othervm -XX:-TieredCompilation -XX:-BackgroundCompilation -XX:-UseOnStackReplacement TestNegMultiply
*
*/

import java.util.Random;

public class TestNegMultiply {
private static Random random = new Random();
Copy link
Member

@TobiHartmann TobiHartmann Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should use Utils.getRandomInstance() from jdk.test.lib.Utils to ensure that the seed is printed for reproducibility. You can check other tests for an example.

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

private static final int TEST_COUNT = 2000;

private static int test(int a, int b) {
return (-a) * (-b);
}

private static void testInt(int a, int b) {
int expected = (-a) * (-b);
Copy link
Contributor

@theRealELiu theRealELiu Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this is the expected value? As the method has been invoked 2000 times, I think it would be compiled by c2.

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default CompileThreshold is 10K when tiered compilation is disabled, which is the case here, so there is no risk.

Copy link
Member

@TobiHartmann TobiHartmann Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why don't you compute expected as a * b?

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to keep as it is to match testxxx() functions. I think it articulates that JIT-ed result matches interpreter's.

for (int i = 0; i < 20_000; i++) {
Copy link
Member

@TobiHartmann TobiHartmann Sep 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need a second loop in here? It's sufficient to set TEST_COUNT high enough to trigger compilation. I would suggest something like this:

private static int testInt(int a, int b) {
    return (-a) * (-b);
}

private static void runIntTests() {
    for (int i = 0; i < TEST_COUNT; i++) {
        int a = random.nextInt();
        int b = random.nextInt();
        int res = testInt(a, b);
        Asserts.assertEQ(a * b, res);
    }
}

And then run with -XX:CompileCommand=dontinline,TestNegMultiply::test*. No need to disable OnStackReplacement.

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inner loop ensures that all tests hit JIT-ed version. If the transformation is broken, I would prefer the test fails for the very first iteration, instead of somewhere in the middle.

I refactored the code to remove inner loop.

Also, fixed command option.

Copy link
Member

@TobiHartmann TobiHartmann Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't control the iteration in which the test would fail if there's a bug in C2 (it could only fail for some random values). Therefore, you could as well use random values for the warmup and simply increase TEST_COUNT to ensure that C2 compilation is triggered and we run a reasonable amount of iterations with C2 compiled code.

Your newest version of the test now has the problem that OSR compilation might C2 compile the computation of the expected value and then you are comparing the output of a C2 compiled method to a C2 compiled method instead of the interpreter. You have the following options:

  • Compute the expected value as a * b. In that case it's fine if the computation is C2 compiled as well.
  • Prevent compilation of the run* methods (either by disabling OSR compilation or by completely disabling compilation of these methods)

Copy link
Member

@TobiHartmann TobiHartmann Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And sorry for being picky here but I would like to keep tests as simple as possible :)

Copy link
Contributor Author

@zhengyu123 zhengyu123 Sep 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed according to you comments.

I really appreciate you suggestions, thanks!

if (expected != test(a, b)) {
throw new RuntimeException("Incorrect result.");
}
}
}

private static long test(long a, long b) {
return (-a) * (-b);
}

private static void testLong(long a, long b) {
long expected = (-a) * (-b);
for (int i = 0; i < 20_000; i++) {
if (expected != test(a, b)) {
throw new RuntimeException("Incorrect result.");
}
}
}
Copy link
Contributor

@theRealELiu theRealELiu Sep 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about calculating the expected value outside the iteration to avoid it to be compiled?

 private static void testLong() {
        for (int i = 0; i < 20_000; i++) {
            long a = random.nextLong();
            long b = random.nextLong();
            long expected = (-a) * (-b);
            if (expected != test(a, b)) {
                throw new RuntimeException("Incorrect result.");
            }
        }
    }

And just call this method once in main to prevent it from being too hot.


private static float test(float a, float b) {
return (-a) * (-b);
}

private static void testFloat(float a, float b) {
float expected = (-a) * (-b);
for (int i = 0; i < 20_000; i++) {
if (expected != test(a, b)) {
throw new RuntimeException("Incorrect result.");
}
}
}

private static double test(double a, double b) {
return (-a) * (-b);
}

private static void testDouble(double a, double b) {
double expected = (-a) * (-b);
for (int i = 0; i < 20_000; i++) {
if (expected != test(a, b)) {
throw new RuntimeException("Incorrect result.");
}
}
}

private static void runIntTests() {
for (int index = 0; index < TEST_COUNT; index ++) {
int a = random.nextInt();
int b = random.nextInt();
testInt(a, b);
}
}

private static void runLongTests() {
for (int index = 0; index < TEST_COUNT; index ++) {
long a = random.nextLong();
long b = random.nextLong();
testLong(a, b);
}
}

private static void runFloatTests() {
for (int index = 0; index < TEST_COUNT; index ++) {
float a = random.nextFloat();
float b = random.nextFloat();
testFloat(a, b);
}
}

private static void runDoubleTests() {
for (int index = 0; index < TEST_COUNT; index ++) {
double a = random.nextDouble();
double b = random.nextDouble();
testDouble(a, b);
}
}

public static void main(String[] args) {
runIntTests();
runLongTests();
runFloatTests();
runDoubleTests();
}
}