Skip to content
Draft
Show file tree
Hide file tree
Changes from all 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
31 changes: 23 additions & 8 deletions src/hotspot/share/opto/subnode.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2069,16 +2069,31 @@ const Type* ReverseLNode::Value(PhaseGVN* phase) const {
return bottom_type();
}

Node* ReverseINode::Identity(PhaseGVN* phase) {
if (in(1)->Opcode() == Op_ReverseI) {
return in(1)->in(1);
static Node* simplify_involution(PhaseGVN* phase, Node* involution) {
if (involution->in(1)->Opcode() == involution->Opcode()) {
Node* original = involution->in(1)->in(1);
const TypeInt *type = phase->type(original)->isa_int();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const TypeInt *type = phase->type(original)->isa_int();
const TypeInt* type = phase->type(original)->isa_int();

// Operations on sub-int types might not be "real" involutions for values outside their type range.
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be helpful to state an example of the disallowed case in the comment, maybe something like:

Suggested change
// Operations on sub-int types might not be "real" involutions for values outside their type range.
// Operations on sub-int types might not be "real" involutions for values outside their type range, for example a ReverseBytesS node with an input larger than short.

// Make sure not to drop potential truncations.
if (type == nullptr || involution->bottom_type()->is_int()->contains(type)) {
return involution->in(1)->in(1);
}
Comment on lines +2076 to +2080
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of skipping the optimization, could you "clean" involution->in(1)->in(1) using mask_int_value()? That would follow the semantics of JVMS§6.5 ireturn.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this would need to happen in Ideal() then instead. Doing that would work too, but it further complicates things with little benefits imo.

}
return this;
return involution;
}

Node* NegNode::Identity(PhaseGVN* phase) {
return simplify_involution(phase, this);
}

Node* ReverseBytesNode::Identity(PhaseGVN* phase) {
return simplify_involution(phase, this);
}

Node* ReverseINode::Identity(PhaseGVN* phase) {
return simplify_involution(phase, this);
}

Node* ReverseLNode::Identity(PhaseGVN* phase) {
if (in(1)->Opcode() == Op_ReverseL) {
return in(1)->in(1);
}
return this;
return simplify_involution(phase, this);
}
2 changes: 2 additions & 0 deletions src/hotspot/share/opto/subnode.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ class NegNode : public Node {
NegNode(Node* in1) : Node(nullptr, in1) {
init_class_id(Class_Neg);
}
virtual Node* Identity(PhaseGVN* phase);
};

//------------------------------NegINode---------------------------------------
Expand Down Expand Up @@ -559,6 +560,7 @@ class ReverseBytesNode : public Node {
public:
ReverseBytesNode(Node* in) : Node(nullptr, in) {}
virtual const Type* Value(PhaseGVN* phase) const;
virtual Node* Identity(PhaseGVN* phase);
};
//-------------------------------ReverseBytesINode--------------------------------
// reverse bytes of an integer
Expand Down
233 changes: 233 additions & 0 deletions test/hotspot/jtreg/compiler/c2/gvn/InvolutionIdentityTests.java
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you not testing involution on NegL/I nodes? Can this not be optimized?

Copy link
Member Author

Choose a reason for hiding this comment

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

NegL/I aren't used currently (see e.g. https://bugs.openjdk.org/browse/JDK-8262346).

Original file line number Diff line number Diff line change
@@ -0,0 +1,233 @@
/*
* Copyright (c) 2025, 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
* 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.
*/
package compiler.c2.gvn;

import compiler.lib.generators.Generator;
import compiler.lib.generators.Generators;
import compiler.lib.generators.RestrictableGenerator;
import compiler.lib.ir_framework.DontCompile;
import compiler.lib.ir_framework.IR;
import compiler.lib.ir_framework.IRNode;
import compiler.lib.ir_framework.Run;
import compiler.lib.ir_framework.Test;
import compiler.lib.ir_framework.TestFramework;
import jdk.test.lib.Asserts;

/*
* @test
* @bug 8350988 8364407
* @summary Test that Identity simplifications of Involution nodes are being performed as expected.
* @library /test/lib /
Comment on lines +39 to +40
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
* @summary Test that Identity simplifications of Involution nodes are being performed as expected.
* @library /test/lib /
* @summary Test that Identity simplifications of Involution nodes are being performed as expected.
* @key randomness
* @library /test/lib /

Since you are using random inputs.

* @compile ReverseBytesConstantsHelper.jasm
* @run driver compiler.c2.gvn.InvolutionIdentityTests
*/
public class InvolutionIdentityTests {

public static final RestrictableGenerator<Integer> GEN_CHAR = Generators.G.safeRestrict(Generators.G.ints(), Character.MIN_VALUE, Character.MAX_VALUE);
public static final RestrictableGenerator<Integer> GEN_SHORT = Generators.G.safeRestrict(Generators.G.ints(), Short.MIN_VALUE, Short.MAX_VALUE);
public static final RestrictableGenerator<Long> GEN_LONG = Generators.G.longs();
public static final RestrictableGenerator<Integer> GEN_INT = Generators.G.ints();
public static final Generator<Float> GEN_FLOAT = Generators.G.floats();
public static final Generator<Double> GEN_DOUBLE = Generators.G.doubles();

public static void main(String[] args) {
TestFramework.runWithFlags("-XX:CompileCommand=inline,compiler.c2.gvn.ReverseBytesConstantsHelper::*");
}

@Run(test = {
"testI1", "testI2",
"testL1", "testL2",
"testS1", "testS2",
"testUS1", "testUS2",
"testF1",
"testD1"
})
public void runMethod() {
int ai = GEN_INT.next();

int mini = Integer.MIN_VALUE;
int maxi = Integer.MAX_VALUE;

assertResultI(0);
assertResultI(ai);
assertResultI(mini);
assertResultI(maxi);

long al = GEN_LONG.next();

long minl = Long.MIN_VALUE;
long maxl = Long.MAX_VALUE;

assertResultL(0);
assertResultL(al);
assertResultL(minl);
assertResultL(maxl);

short as = GEN_SHORT.next().shortValue();

short mins = Short.MIN_VALUE;
short maxs = Short.MAX_VALUE;

assertResultS((short) 0);
assertResultS(as);
assertResultS(mins);
assertResultS(maxs);

char ac = (char) GEN_CHAR.next().intValue();

char minc = Character.MIN_VALUE;
char maxc = Character.MAX_VALUE;

assertResultUS((char) 0);
assertResultUS(ac);
assertResultUS(minc);
assertResultUS(maxc);

// short and char variants but passing an int
assertResultTruncating(0);
assertResultTruncating(ai);
assertResultTruncating(mini);
assertResultTruncating(maxi);

float af = GEN_FLOAT.next();
float inf = Float.POSITIVE_INFINITY;
float nanf = Float.NaN;

assertResultF(0f);
assertResultF(-0f);
assertResultF(af);
assertResultF(inf);
assertResultF(nanf);

double ad = GEN_DOUBLE.next();
double ind = Double.POSITIVE_INFINITY;
double nand = Double.NaN;

assertResultD(0d);
assertResultD(-0d);
assertResultD(ad);
assertResultD(ind);
assertResultD(nand);

}
Comment on lines +131 to +132
Copy link
Member

Choose a reason for hiding this comment

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

There's an extra whitespace:

Suggested change
}
}


@DontCompile
public void assertResultI(int a) {
Asserts.assertEQ(Integer.reverseBytes(Integer.reverseBytes(a)), testI1(a));
Asserts.assertEQ(Integer.reverse(Integer.reverse(a)) , testI2(a));
}

@DontCompile
public void assertResultL(long a) {
Asserts.assertEQ(Long.reverseBytes(Long.reverseBytes(a)), testL1(a));
Asserts.assertEQ(Long.reverse(Long.reverse(a)) , testL2(a));
}

@DontCompile
public void assertResultS(short a) {
Asserts.assertEQ(Short.reverseBytes(Short.reverseBytes(a)), testS1(a));
}

@DontCompile
public void assertResultUS(char a) {
Asserts.assertEQ(Character.reverseBytes(Character.reverseBytes(a)), testUS1(a));
}

@DontCompile
public void assertResultF(float a) {
Asserts.assertEQ(Float.floatToRawIntBits(-(-a)), Float.floatToRawIntBits(testF1(a)));
}

@DontCompile
public void assertResultD(double a) {
Asserts.assertEQ(Double.doubleToRawLongBits(-(-a)), Double.doubleToRawLongBits(testD1(a)));
}

@DontCompile
public void assertResultTruncating(int a) {
Asserts.assertEQ((int) Short.reverseBytes( ReverseBytesConstantsHelper.reverseBytesShort(a)), testS2( a));
Asserts.assertEQ((int) Character.reverseBytes(ReverseBytesConstantsHelper.reverseBytesChar( a)), testUS2(a));
}

@Test
@IR(failOn = {IRNode.REVERSE_BYTES_I})
public int testI1(int x) {
return Integer.reverseBytes(Integer.reverseBytes(x));
}

@Test
@IR(failOn = {IRNode.REVERSE_I})
public int testI2(int x) {
return Integer.reverse(Integer.reverse(x));
}

@Test
@IR(failOn = {IRNode.REVERSE_BYTES_L})
public long testL1(long x) {
return Long.reverseBytes(Long.reverseBytes(x));
}

@Test
@IR(failOn = {IRNode.REVERSE_L})
public long testL2(long x) {
return Long.reverse(Long.reverse(x));
}

@Test
@IR(failOn = {IRNode.REVERSE_BYTES_S})
public short testS1(short x) {
// explicit cast to restrict the input to TypeInt::SHORT
return Short.reverseBytes(Short.reverseBytes((short) (int) x));
}

@Test
@IR(failOn = {IRNode.REVERSE_BYTES_US})
public char testUS1(char x) {
// explicit cast to restrict the input to TypeInt::CHAR
return Character.reverseBytes(Character.reverseBytes((char) (int) x));
}

@Test
@IR(counts = {IRNode.REVERSE_BYTES_S, "2"})
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this is fine. The intrinsics might not apply to all platforms, in which case this would fail I think?

Copy link
Contributor

Choose a reason for hiding this comment

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

The only case I can immediately think of is riscv without -XX:+UseZbb. But you can easily disable the test for that platform.

public int testS2(int x) {
return Short.reverseBytes(ReverseBytesConstantsHelper.reverseBytesShort(x));
}

@Test
@IR(counts = {IRNode.REVERSE_BYTES_US, "2"})
public int testUS2(int x) {
return Character.reverseBytes(ReverseBytesConstantsHelper.reverseBytesChar(x));
}

@Test
@IR(failOn = {IRNode.NEG_F})
public float testF1(float x) {
return -(-x);
}

@Test
@IR(failOn = {IRNode.NEG_D})
public double testD1(double x) {
return -(-x);
}
}
10 changes: 10 additions & 0 deletions test/hotspot/jtreg/compiler/lib/ir_framework/IRNode.java
Original file line number Diff line number Diff line change
Expand Up @@ -1513,6 +1513,16 @@ public class IRNode {
superWordNodes(MAX_REDUCTION_V, "MaxReductionV");
}

public static final String NEG_F = PREFIX + "NEG_F" + POSTFIX;
static {
beforeMatchingNameRegex(NEG_F, "NegF");
}

public static final String NEG_D = PREFIX + "NEG_D" + POSTFIX;
static {
beforeMatchingNameRegex(NEG_D, "NegD");
}

public static final String NEG_VF = VECTOR_PREFIX + "NEG_VF" + POSTFIX;
static {
vectorNode(NEG_VF, "NegVF", TYPE_FLOAT);
Expand Down