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

8276673: Optimize abs operations in C2 compiler #6755

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions src/hotspot/share/opto/subnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1839,6 +1839,72 @@ bool BoolNode::is_counted_loop_exit_test() {
return false;
}

//=============================================================================
//------------------------------Value------------------------------------------
const Type* AbsNode::Value(PhaseGVN* phase) const {
const Type* t1 = phase->type(in(1));
if (t1 == Type::TOP) return Type::TOP;

switch (t1->base()) {
case Type::Int: {
const TypeInt* ti = t1->is_int();
if (ti->is_con()) {
// Special case for min_jint: Math.abs(min_jint) = min_jint.
// Do not use C++ abs() for min_jint to avoid undefined behavior.
return (ti->is_con(min_jint)) ? TypeInt::MIN : TypeInt::make(abs(ti->get_con()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return (ti->is_con(min_jint)) ? TypeInt::MIN : TypeInt::make(abs(ti->get_con()));
return TypeInt::make(uabs(ti->get_con());

We have uabs() for julong and unsigned int.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for your review. Fixed.

}
break;
}
case Type::Long: {
const TypeLong* tl = t1->is_long();
if (tl->is_con()) {
// Special case for min_jlong: Math.abs(min_jlong) = min_jlong.
// Do not use C++ abs() for min_jlong to avoid undefined behavior.
return (tl->is_con(min_jlong)) ? TypeLong::MIN : TypeLong::make(abs(tl->get_con()));
}
break;
}
case Type::FloatCon:
return TypeF::make(abs(t1->getf()));
case Type::DoubleCon:
return TypeD::make(abs(t1->getd()));
default:
break;
}

return bottom_type();
}

//------------------------------Identity----------------------------------------
Node* AbsNode::Identity(PhaseGVN* phase) {
Node* in1 = in(1);
// No need to do abs for non-negative values
if (phase->type(in1)->higher_equal(TypeInt::POS) ||
phase->type(in1)->higher_equal(TypeLong::POS)) {
return in1;
}
// Convert "abs(abs(x))" into "abs(x)"
if (in1->Opcode() == Opcode()) {
return in1;
}
return this;
}

//------------------------------Ideal------------------------------------------
Node* AbsNode::Ideal(PhaseGVN* phase, bool can_reshape) {
Node* in1 = in(1);
// Convert "abs(0-x)" into "abs(x)"
if (in1->is_Sub() && phase->type(in1->in(1))->is_zero_type()) {
set_req(1, in1->in(2));
PhaseIterGVN* igvn = phase->is_IterGVN();
if (igvn) {
igvn->_worklist.push(in1);
Copy link
Member

Choose a reason for hiding this comment

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

Why is that needed? Because in1 could become dead? You should use set_req_X above.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Thanks.

}
return this;
}
return NULL;
}

//=============================================================================
//------------------------------Value------------------------------------------
// Compute sqrt
Expand Down
5 changes: 4 additions & 1 deletion src/hotspot/share/opto/subnode.hpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1997, 2019, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1997, 2021, Oracle and/or its affiliates. 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
Expand Down Expand Up @@ -347,6 +347,9 @@ class BoolNode : public Node {
class AbsNode : public Node {
public:
AbsNode( Node *value ) : Node(0,value) {}
virtual Node* Identity(PhaseGVN* phase);
virtual Node* Ideal(PhaseGVN* phase, bool can_reshape);
virtual const Type* Value(PhaseGVN* phase) const;
};

//------------------------------AbsINode---------------------------------------
Expand Down
4 changes: 2 additions & 2 deletions src/hotspot/share/opto/type.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ class TypeInt : public TypeInteger {

// Check for single integer
bool is_con() const { return _lo==_hi; }
bool is_con(int i) const { return is_con() && _lo == i; }
bool is_con(jint i) const { return is_con() && _lo == i; }
jint get_con() const { assert(is_con(), "" ); return _lo; }

virtual bool is_finite() const; // Has a finite value
Expand Down Expand Up @@ -661,7 +661,7 @@ class TypeLong : public TypeInteger {

// Check for single integer
bool is_con() const { return _lo==_hi; }
bool is_con(int i) const { return is_con() && _lo == i; }
bool is_con(jlong i) const { return is_con() && _lo == i; }
jlong get_con() const { assert(is_con(), "" ); return _lo; }

// Check for positive 32-bit value.
Expand Down
155 changes: 151 additions & 4 deletions test/hotspot/jtreg/compiler/c2/TestAbs.java
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,151 @@
* questions.
*/

package compiler.c2;

/*
* @test
* @bug 8248445
* @summary Use of AbsI / AbsL nodes should be limited to supported platforms
* @bug 8248445 8276673
* @summary Abs nodes detection and optimization in C2
* @library /test/lib
* @requires vm.debug == true
*
* @run main/othervm -XX:-TieredCompilation -Xbatch -XX:CompileOnly=java.lang.Math::abs compiler.c2.TestAbs
* @run main/othervm -XX:-TieredCompilation compiler.c2.TestAbs
*/

package compiler.c2;
import jdk.test.lib.Asserts;

public class TestAbs {
private static int SIZE = 500;
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

Copy link
Author

Choose a reason for hiding this comment

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

Done. Thanks.


public static char [] cspecial = {
0, 42, 128, 256, 1024, 4096, 65535
};

public static int [] ispecial = {
0, Integer.MAX_VALUE, Integer.MIN_VALUE, -42, 42, -1, 1
};

public static long [] lspecial = {
0, Long.MAX_VALUE, Long.MIN_VALUE, -42, 42, -1, 1
};

public static float [] fspecial = {
0.0f,
-0.0f,
Float.MAX_VALUE,
Float.MIN_VALUE,
-Float.MAX_VALUE,
-Float.MIN_VALUE,
Float.NaN,
Float.POSITIVE_INFINITY,
Float.NEGATIVE_INFINITY,
Integer.MAX_VALUE,
Integer.MIN_VALUE,
Long.MAX_VALUE,
Long.MIN_VALUE,
-1.0f,
1.0f,
-42.0f,
42.0f
};

public static double [] dspecial = {
0.0,
-0.0,
Double.MAX_VALUE,
Double.MIN_VALUE,
-Double.MAX_VALUE,
-Double.MIN_VALUE,
Double.NaN,
Double.POSITIVE_INFINITY,
Double.NEGATIVE_INFINITY,
Integer.MAX_VALUE,
Integer.MIN_VALUE,
Long.MIN_VALUE,
Long.MAX_VALUE,
-1,
1,
42,
-42,
Math.PI,
Math.E,
Float.MAX_VALUE,
Float.MIN_VALUE,
-Float.MAX_VALUE,
-Float.MIN_VALUE,
Float.NaN,
Float.POSITIVE_INFINITY,
Float.NEGATIVE_INFINITY
};

public static void testAbsConstant() {
// Test abs(constant) optimization for int
Asserts.assertEquals(Integer.MAX_VALUE, Math.abs(Integer.MAX_VALUE));
Asserts.assertEquals(Integer.MIN_VALUE, Math.abs(Integer.MIN_VALUE));
Asserts.assertEquals(Integer.MAX_VALUE, Math.abs(-Integer.MAX_VALUE));

// Test abs(constant) optimization for long
Asserts.assertEquals(Long.MAX_VALUE, Math.abs(Long.MAX_VALUE));
Asserts.assertEquals(Long.MIN_VALUE, Math.abs(Long.MIN_VALUE));
Asserts.assertEquals(Long.MAX_VALUE, Math.abs(-Long.MAX_VALUE));

// Test abs(constant) optimization for float
Asserts.assertEquals(Float.NaN, Math.abs(Float.NaN));
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest something like:

assertTrue(Float.isNaN(Math.abs(Float.NaN)))

Copy link
Author

Choose a reason for hiding this comment

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

Done. Thanks.

Asserts.assertEquals(Float.POSITIVE_INFINITY, Math.abs(Float.NEGATIVE_INFINITY));
Asserts.assertEquals(Float.POSITIVE_INFINITY, Math.abs(Float.POSITIVE_INFINITY));
Asserts.assertEquals(0.0f, Math.abs(0.0f));
Asserts.assertEquals(0.0f, Math.abs(-0.0f));
Asserts.assertEquals(Float.MAX_VALUE, Math.abs(Float.MAX_VALUE));
Asserts.assertEquals(Float.MIN_VALUE, Math.abs(Float.MIN_VALUE));
Asserts.assertEquals(Float.MAX_VALUE, Math.abs(-Float.MAX_VALUE));
Asserts.assertEquals(Float.MIN_VALUE, Math.abs(-Float.MIN_VALUE));

// Test abs(constant) optimization for double
Asserts.assertEquals(Double.NaN, Math.abs(Double.NaN));
Asserts.assertEquals(Double.POSITIVE_INFINITY, Math.abs(Double.NEGATIVE_INFINITY));
Asserts.assertEquals(Double.POSITIVE_INFINITY, Math.abs(Double.POSITIVE_INFINITY));
Asserts.assertEquals(0.0, Math.abs(0.0));
Asserts.assertEquals(0.0, Math.abs(-0.0));
Asserts.assertEquals(Double.MAX_VALUE, Math.abs(Double.MAX_VALUE));
Asserts.assertEquals(Double.MIN_VALUE, Math.abs(Double.MIN_VALUE));
Asserts.assertEquals(Double.MAX_VALUE, Math.abs(-Double.MAX_VALUE));
Asserts.assertEquals(Double.MIN_VALUE, Math.abs(-Double.MIN_VALUE));
}

private static void testAbsTransformInt(int[] a) {
Copy link
Member

Choose a reason for hiding this comment

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

If you want to verify C2's transformation, probably we should use C2's IR test framework.

Copy link
Author

Choose a reason for hiding this comment

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

Done. Thanks.

for (int i = 0; i < a.length; i++) {
Asserts.assertEquals(Math.abs(Math.abs(a[i])), Math.abs(a[i]));
Asserts.assertEquals(Math.abs(0 - a[i]), Math.abs(a[i]));
}
}

private static void testAbsTransformLong(long[] a) {
for (int i = 0; i < a.length; i++) {
Asserts.assertEquals(Math.abs(Math.abs(a[i])), Math.abs(a[i]));
Asserts.assertEquals(Math.abs(0 - a[i]), Math.abs(a[i]));
}
}

private static void testAbsTransformFloat(float[] a) {
for (int i = 0; i < a.length; i++) {
Asserts.assertEquals(Math.abs(Math.abs(a[i])), Math.abs(a[i]));
Asserts.assertEquals(Math.abs(0 - a[i]), Math.abs(a[i]));
}
}

private static void testAbsTransformDouble(double[] a) {
for (int i = 0; i < a.length; i++) {
Asserts.assertEquals(Math.abs(Math.abs(a[i])), Math.abs(a[i]));
Asserts.assertEquals(Math.abs(0 - a[i]), Math.abs(a[i]));
}
}

private static void testAbsOptChar(char[] a) {
for (int i = 0; i < a.length; i++) {
Asserts.assertEquals(a[i], (char) Math.abs(a[i]));
}
}

public static void test() {
// java.lang.Math.abs() collapses into AbsI/AbsL nodes on platforms that support the correspondent nodes
Expand All @@ -45,7 +179,20 @@ public static void test() {
public static void main(String args[]) {
for (int i = 0; i < 20_000; i++) {
test();

testAbsConstant();

// Verify abs(abs(x)) = abs(x) for all types
// Verify abs(0-x) = abs(x) for all types
testAbsTransformInt(ispecial);
testAbsTransformLong(lspecial);
testAbsTransformFloat(fspecial);
testAbsTransformDouble(dspecial);

// Verify abs(non-negative_value) = non-negative_value
testAbsOptChar(cspecial);
}

}
}

5 changes: 5 additions & 0 deletions test/micro/org/openjdk/bench/java/lang/MathBench.java
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ public long absLong() {
return Math.abs(long13);
}

@Benchmark
public int absConstantInt() {
return Math.abs(-3);
}

@Benchmark
public double acosDouble() {
return Math.acos(double1);
Expand Down