Skip to content

Commit 868d594

Browse files
mur47x111mur47x111
committed
[GR-33972] NarrowNode.getReverseOp now returns ZeroExtend or SignExtend accordingly.
PullRequest: graal/9909
2 parents 334886f + 1ebc25b commit 868d594

File tree

9 files changed

+241
-29
lines changed

9 files changed

+241
-29
lines changed

‎compiler/src/org.graalvm.compiler.core.aarch64/src/org/graalvm/compiler/core/aarch64/AArch64NodeMatchRules.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2015, 2021, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2015, 2022, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -155,7 +155,7 @@ protected AArch64Kind getMemoryKind(MemoryAccess access) {
155155
return (AArch64Kind) gen.getLIRKind(((ValueNode) access).stamp(NodeView.DEFAULT)).getPlatformKind();
156156
}
157157

158-
private static boolean isSupportedExtendedAddSubShift(IntegerConvertNode<?, ?> node, int clampedShiftAmt) {
158+
private static boolean isSupportedExtendedAddSubShift(IntegerConvertNode<?> node, int clampedShiftAmt) {
159159
assert clampedShiftAmt >= 0;
160160
if (clampedShiftAmt <= 4) {
161161
switch (node.getInputBits()) {
@@ -317,7 +317,7 @@ private ComplexMatchResult emitExtendedAddSubShift(BinaryNode op, ValueNode x, V
317317
public ComplexMatchResult mergeSignExtendByShiftIntoAddSub(BinaryNode op, LeftShiftNode lshift, ValueNode ext, ValueNode x, ValueNode y) {
318318
assert isNumericInteger(lshift);
319319
int shiftAmt = getClampedShiftAmt(lshift);
320-
if (!isSupportedExtendedAddSubShift((IntegerConvertNode<?, ?>) ext, shiftAmt)) {
320+
if (!isSupportedExtendedAddSubShift((IntegerConvertNode<?>) ext, shiftAmt)) {
321321
return null;
322322
}
323323
ExtendType extType;
@@ -885,7 +885,7 @@ public ComplexMatchResult mergeDowncastIntoAddSub(BinaryNode op, ValueNode x, Va
885885
@MatchRule("(Add=op x (ZeroExtend=ext y))")
886886
@MatchRule("(Sub=op x (ZeroExtend=ext y))")
887887
public ComplexMatchResult mergeSignExtendIntoAddSub(BinaryNode op, UnaryNode ext, ValueNode x, ValueNode y) {
888-
if (!isSupportedExtendedAddSubShift((IntegerConvertNode<?, ?>) ext, 0)) {
888+
if (!isSupportedExtendedAddSubShift((IntegerConvertNode<?>) ext, 0)) {
889889
return null;
890890
}
891891

‎compiler/src/org.graalvm.compiler.nodes.test/src/org/graalvm/compiler/nodes/test/NarrowPreservesOrderTest.java

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
/*
2+
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation. Oracle designates this
8+
* particular file as subject to the "Classpath" exception as provided
9+
* by Oracle in the LICENSE file that accompanied this code.
10+
*
11+
* This code is distributed in the hope that it will be useful, but WITHOUT
12+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
13+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
14+
* version 2 for more details (a copy is included in the LICENSE file that
15+
* accompanied this code).
16+
*
17+
* You should have received a copy of the GNU General Public License version
18+
* 2 along with this work; if not, write to the Free Software Foundation,
19+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
20+
*
21+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
22+
* or visit www.oracle.com if you need additional information or have any
23+
* questions.
24+
*/
25+
package org.graalvm.compiler.nodes.test;
26+
27+
import static org.graalvm.compiler.core.common.calc.CanonicalCondition.BT;
28+
import static org.graalvm.compiler.core.common.calc.CanonicalCondition.LT;
29+
import static org.junit.Assert.assertEquals;
30+
import static org.junit.Assert.assertTrue;
31+
32+
import org.graalvm.compiler.core.common.NumUtil;
33+
import org.graalvm.compiler.core.common.calc.CanonicalCondition;
34+
import org.graalvm.compiler.core.common.type.IntegerStamp;
35+
import org.graalvm.compiler.core.common.type.Stamp;
36+
import org.graalvm.compiler.core.common.type.StampFactory;
37+
import org.graalvm.compiler.core.common.type.StampPair;
38+
import org.graalvm.compiler.graph.test.GraphTest;
39+
import org.graalvm.compiler.nodes.ParameterNode;
40+
import org.graalvm.compiler.nodes.calc.NarrowNode;
41+
import org.junit.Test;
42+
43+
import jdk.vm.ci.code.CodeUtil;
44+
import jdk.vm.ci.meta.JavaKind;
45+
46+
/**
47+
* This class tests that {@link NarrowNode#preservesOrder(CanonicalCondition)} returns correct
48+
* value.
49+
*/
50+
public class NarrowPreservesOrderTest extends GraphTest {
51+
52+
private static IntegerStamp signExtend(Stamp stamp, int bits) {
53+
assertTrue(stamp instanceof IntegerStamp);
54+
IntegerStamp integerStamp = (IntegerStamp) stamp;
55+
return IntegerStamp.create(bits, integerStamp.lowerBound(), integerStamp.upperBound());
56+
}
57+
58+
private static IntegerStamp zeroExtend(Stamp stamp, int bits) {
59+
assertTrue(stamp instanceof IntegerStamp);
60+
IntegerStamp integerStamp = (IntegerStamp) stamp;
61+
return IntegerStamp.create(bits, integerStamp.unsignedLowerBound(), integerStamp.unsignedUpperBound());
62+
}
63+
64+
private static IntegerStamp forConstantInt(long cst) {
65+
return IntegerStamp.create(32, cst, cst);
66+
}
67+
68+
private static IntegerStamp forConstantLong(long cst) {
69+
return IntegerStamp.create(64, cst, cst);
70+
}
71+
72+
private static void testPreserveOrder(Stamp inputStamp, int resultBits, CanonicalCondition cond, boolean expected) {
73+
ParameterNode input = new ParameterNode(0, StampPair.createSingle(inputStamp));
74+
NarrowNode narrow = new NarrowNode(input, resultBits);
75+
assertEquals(expected, narrow.preservesOrder(cond));
76+
}
77+
78+
@Test
79+
public void testBoolean() {
80+
testPreserveOrder(forConstantInt(0), 1, LT, true);
81+
testPreserveOrder(forConstantInt(0), 1, BT, true);
82+
testPreserveOrder(forConstantInt(1), 1, LT, false);
83+
testPreserveOrder(forConstantInt(1), 1, BT, true);
84+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Boolean), 32), 1, LT, false);
85+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Boolean), 32), 1, BT, true);
86+
87+
testPreserveOrder(StampFactory.forKind(JavaKind.Byte), 1, LT, false);
88+
testPreserveOrder(StampFactory.forKind(JavaKind.Byte), 1, BT, false);
89+
}
90+
91+
@Test
92+
public void testByte() {
93+
testPreserveOrder(forConstantInt(0), 8, LT, true);
94+
testPreserveOrder(forConstantInt(0), 8, BT, true);
95+
testPreserveOrder(forConstantInt(CodeUtil.maxValue(8)), 8, LT, true);
96+
testPreserveOrder(forConstantInt(CodeUtil.maxValue(8)), 8, BT, true);
97+
testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(8)), 8, LT, false);
98+
testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(8)), 8, BT, true);
99+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Byte), 32), 8, LT, true);
100+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Byte), 32), 8, BT, true);
101+
testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(8), 32), 8, LT, false);
102+
testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(8), 32), 8, BT, true);
103+
104+
testPreserveOrder(StampFactory.forKind(JavaKind.Short), 8, LT, false);
105+
testPreserveOrder(StampFactory.forKind(JavaKind.Short), 8, BT, false);
106+
}
107+
108+
@Test
109+
public void testShort() {
110+
testPreserveOrder(forConstantInt(0), 16, LT, true);
111+
testPreserveOrder(forConstantInt(0), 16, BT, true);
112+
testPreserveOrder(forConstantInt(CodeUtil.maxValue(16)), 16, LT, true);
113+
testPreserveOrder(forConstantInt(CodeUtil.maxValue(16)), 16, BT, true);
114+
testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(16)), 16, LT, false);
115+
testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(16)), 16, BT, true);
116+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Short), 32), 16, LT, true);
117+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Short), 32), 16, BT, true);
118+
testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(16), 32), 16, LT, false);
119+
testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(16), 32), 16, BT, true);
120+
121+
testPreserveOrder(StampFactory.forKind(JavaKind.Int), 16, LT, false);
122+
testPreserveOrder(StampFactory.forKind(JavaKind.Int), 16, BT, false);
123+
}
124+
125+
@Test
126+
public void testInt() {
127+
testPreserveOrder(forConstantLong(0), 32, LT, true);
128+
testPreserveOrder(forConstantLong(0), 32, BT, true);
129+
testPreserveOrder(forConstantLong(CodeUtil.maxValue(32)), 32, LT, true);
130+
testPreserveOrder(forConstantLong(CodeUtil.maxValue(32)), 32, BT, true);
131+
testPreserveOrder(forConstantLong(NumUtil.maxValueUnsigned(32)), 32, LT, false);
132+
testPreserveOrder(forConstantLong(NumUtil.maxValueUnsigned(32)), 32, BT, true);
133+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Int), 64), 32, LT, true);
134+
testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Int), 64), 32, BT, true);
135+
testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(32), 64), 32, LT, false);
136+
testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(32), 64), 32, BT, true);
137+
138+
testPreserveOrder(StampFactory.forKind(JavaKind.Long), 32, LT, false);
139+
testPreserveOrder(StampFactory.forKind(JavaKind.Long), 32, BT, false);
140+
}
141+
}

‎compiler/src/org.graalvm.compiler.nodes.test/src/org/graalvm/compiler/nodes/test/NarrowTest.java

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
/*
2+
* Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
3+
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
4+
*
5+
* This code is free software; you can redistribute it and/or modify it
6+
* under the terms of the GNU General Public License version 2 only, as
7+
* published by the Free Software Foundation. Oracle designates this
8+
* particular file as subject to the "Classpath" exception as provided
9+
* by Oracle in the LICENSE file that accompanied this code.
10+
*
11+
* This code is distributed in the hope that it will be useful, but WITHOUT
12+
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
13+
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
14+
* version 2 for more details (a copy is included in the LICENSE file that
15+
* accompanied this code).
16+
*
17+
* You should have received a copy of the GNU General Public License version
18+
* 2 along with this work; if not, write to the Free Software Foundation,
19+
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
20+
*
21+
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
22+
* or visit www.oracle.com if you need additional information or have any
23+
* questions.
24+
*/
25+
package org.graalvm.compiler.nodes.test;
26+
27+
import org.graalvm.compiler.api.directives.GraalDirectives;
28+
import org.graalvm.compiler.core.test.GraalCompilerTest;
29+
import org.junit.Test;
30+
31+
public class NarrowTest extends GraalCompilerTest {
32+
33+
@Test
34+
public void testSnippet0() {
35+
test("snippet0", (char) 0);
36+
}
37+
38+
static boolean snippet0(char c0) {
39+
return -2 > (byte) ((byte) c0 / (byte) 2134864769);
40+
}
41+
42+
@Test
43+
public void testSnippet1() {
44+
test("snippet1", 50400L);
45+
}
46+
47+
static boolean snippet1(long l0) {
48+
if ((char) ((byte) l0) >= Character.MAX_VALUE) {
49+
return true;
50+
}
51+
GraalDirectives.blackhole((byte) l0);
52+
return false;
53+
}
54+
}

‎compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/AddNode.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2011, 2020, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2011, 2022, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -111,7 +111,7 @@ private static ValueNode canonical(AddNode addNode, BinaryOp<Add> op, ValueNode
111111

112112
// Attempt to optimize the pattern of an extend node between two add nodes.
113113
if (c instanceof JavaConstant && (forX instanceof SignExtendNode || forX instanceof ZeroExtendNode)) {
114-
IntegerConvertNode<?, ?> integerConvertNode = (IntegerConvertNode<?, ?>) forX;
114+
IntegerConvertNode<?> integerConvertNode = (IntegerConvertNode<?>) forX;
115115
ValueNode valueNode = integerConvertNode.getValue();
116116
long constant = ((JavaConstant) c).asLong();
117117
if (valueNode instanceof AddNode) {

‎compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/IntegerConvertNode.java

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2014, 2019, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -50,13 +50,13 @@
5050
* An {@code IntegerConvert} converts an integer to an integer of different width.
5151
*/
5252
@NodeInfo
53-
public abstract class IntegerConvertNode<OP, REV> extends UnaryNode implements ArithmeticOperation, ConvertNode, ArithmeticLIRLowerable, StampInverter {
53+
public abstract class IntegerConvertNode<OP> extends UnaryNode implements ArithmeticOperation, ConvertNode, ArithmeticLIRLowerable, StampInverter {
5454
@SuppressWarnings("rawtypes") public static final NodeClass<IntegerConvertNode> TYPE = NodeClass.create(IntegerConvertNode.class);
5555

5656
protected final int inputBits;
5757
protected final int resultBits;
5858

59-
protected IntegerConvertNode(NodeClass<? extends IntegerConvertNode<OP, REV>> c, IntegerConvertOp<OP> opForStampComputation, int inputBits, int resultBits, ValueNode input) {
59+
protected IntegerConvertNode(NodeClass<? extends IntegerConvertNode<OP>> c, IntegerConvertOp<OP> opForStampComputation, int inputBits, int resultBits, ValueNode input) {
6060
super(c, opForStampComputation.foldStamp(inputBits, resultBits, input.stamp(NodeView.DEFAULT)), input);
6161
this.inputBits = inputBits;
6262
this.resultBits = resultBits;
@@ -73,7 +73,7 @@ public int getResultBits() {
7373

7474
protected abstract IntegerConvertOp<OP> getOp(ArithmeticOpTable table);
7575

76-
protected abstract IntegerConvertOp<REV> getReverseOp(ArithmeticOpTable table);
76+
protected abstract IntegerConvertOp<?> getReverseOp(ArithmeticOpTable table);
7777

7878
@Override
7979
public final IntegerConvertOp<OP> getArithmeticOp() {
@@ -87,7 +87,7 @@ public Constant convert(Constant c, ConstantReflectionProvider constantReflectio
8787

8888
@Override
8989
public Constant reverse(Constant c, ConstantReflectionProvider constantReflection) {
90-
IntegerConvertOp<REV> reverse = getReverseOp(ArithmeticOpTable.forStamp(stamp(NodeView.DEFAULT)));
90+
IntegerConvertOp<?> reverse = getReverseOp(ArithmeticOpTable.forStamp(stamp(NodeView.DEFAULT)));
9191
return reverse.foldConstant(getResultBits(), getInputBits(), c);
9292
}
9393

‎compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/NarrowNode.java

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2014, 2021, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2014, 2022, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* This code is free software; you can redistribute it and/or modify it
@@ -34,12 +34,13 @@
3434
import org.graalvm.compiler.core.common.type.IntegerStamp;
3535
import org.graalvm.compiler.core.common.type.PrimitiveStamp;
3636
import org.graalvm.compiler.core.common.type.Stamp;
37+
import org.graalvm.compiler.debug.GraalError;
3738
import org.graalvm.compiler.graph.NodeClass;
38-
import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
3939
import org.graalvm.compiler.lir.gen.ArithmeticLIRGeneratorTool;
4040
import org.graalvm.compiler.nodeinfo.NodeInfo;
4141
import org.graalvm.compiler.nodes.NodeView;
4242
import org.graalvm.compiler.nodes.ValueNode;
43+
import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
4344
import org.graalvm.compiler.nodes.spi.NodeLIRBuilderTool;
4445

4546
import jdk.vm.ci.code.CodeUtil;
@@ -48,7 +49,7 @@
4849
* The {@code NarrowNode} converts an integer to a narrower integer.
4950
*/
5051
@NodeInfo(cycles = CYCLES_1)
51-
public final class NarrowNode extends IntegerConvertNode<Narrow, IntegerConvertOp.ZeroExtend> {
52+
public final class NarrowNode extends IntegerConvertNode<Narrow> {
5253

5354
public static final NodeClass<NarrowNode> TYPE = NodeClass.create(NarrowNode.class);
5455

@@ -81,25 +82,38 @@ protected IntegerConvertOp<Narrow> getOp(ArithmeticOpTable table) {
8182
}
8283

8384
@Override
84-
protected IntegerConvertOp<IntegerConvertOp.ZeroExtend> getReverseOp(ArithmeticOpTable table) {
85-
return table.getZeroExtend();
85+
protected IntegerConvertOp<?> getReverseOp(ArithmeticOpTable table) {
86+
assert isSignedLossless() || isUnsignedLossless();
87+
return isSignedLossless() ? table.getSignExtend() : table.getZeroExtend();
8688
}
8789

8890
@Override
8991
public boolean isLossless() {
90-
return checkLossless(this.getResultBits(), this.getValue());
92+
// This is conservative as we don't know which compare operator is being used.
93+
return isSignedLossless() && isUnsignedLossless();
9194
}
9295

93-
public static boolean checkLossless(int bits, ValueNode value) {
96+
private boolean isSignedLossless() {
9497
Stamp valueStamp = value.stamp(NodeView.DEFAULT);
98+
int bits = getResultBits();
9599
if (bits > 0 && valueStamp instanceof IntegerStamp) {
96100
IntegerStamp integerStamp = (IntegerStamp) valueStamp;
97101
long bitsRangeMin = CodeUtil.minValue(bits);
98102
long bitsRangeMax = CodeUtil.maxValue(bits);
99103
if (bitsRangeMin <= integerStamp.lowerBound() && integerStamp.upperBound() <= bitsRangeMax) {
100104
// all signed values fit
101105
return true;
102-
} else if (integerStamp.isPositive()) {
106+
}
107+
}
108+
return false;
109+
}
110+
111+
private boolean isUnsignedLossless() {
112+
Stamp valueStamp = value.stamp(NodeView.DEFAULT);
113+
int bits = getResultBits();
114+
if (bits > 0 && valueStamp instanceof IntegerStamp) {
115+
IntegerStamp integerStamp = (IntegerStamp) valueStamp;
116+
if (integerStamp.isPositive()) {
103117
long valueUpMask = integerStamp.upMask();
104118
if ((valueUpMask & CodeUtil.mask(bits)) == valueUpMask) {
105119
// value is unsigned and fits
@@ -114,10 +128,13 @@ public static boolean checkLossless(int bits, ValueNode value) {
114128
public boolean preservesOrder(CanonicalCondition cond) {
115129
switch (cond) {
116130
case LT:
117-
// Must guarantee that also sign bit does not flip.
118-
return checkLossless(this.getResultBits() - 1, this.getValue());
131+
return isSignedLossless();
132+
case EQ:
133+
case BT:
134+
// We may use signed stamps to represent unsigned integers.
135+
return isSignedLossless() || isUnsignedLossless();
119136
default:
120-
return checkLossless(this.getResultBits(), this.getValue());
137+
throw GraalError.shouldNotReachHere("Unsupported canonical condition.");
121138
}
122139
}
123140

@@ -136,7 +153,7 @@ public ValueNode canonical(CanonicalizerTool tool, ValueNode forValue) {
136153
return new NarrowNode(other.getValue(), other.getInputBits(), getResultBits());
137154
} else if (forValue instanceof IntegerConvertNode) {
138155
// SignExtendNode or ZeroExtendNode
139-
IntegerConvertNode<?, ?> other = (IntegerConvertNode<?, ?>) forValue;
156+
IntegerConvertNode<?> other = (IntegerConvertNode<?>) forValue;
140157
if (other.getValue().hasExactlyOneUsage() && other.hasMoreThanOneUsage()) {
141158
// Do not perform if this will introduce a new live value.
142159
// If the original value's usage count is > 1, there is already another user.

0 commit comments

Comments
 (0)