Skip to content

Commit 9378d67

Browse files
gergo-gergo-
committed
[GR-41654] Mark multiply-high nodes as not associative
PullRequest: graal/12902
2 parents 3ef7602 + f1da457 commit 9378d67

File tree

5 files changed

+81
-6
lines changed

5 files changed

+81
-6
lines changed

‎compiler/src/org.graalvm.compiler.core.common/src/org/graalvm/compiler/core/common/type/IntegerStamp.java

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1012,7 +1012,16 @@ public boolean isNeutral(Constant value) {
10121012
}
10131013
},
10141014

1015-
new BinaryOp.MulHigh(true, true) {
1015+
/*
1016+
* MulHigh is not associative, for example:
1017+
*
1018+
* mulHigh(mulHigh(-1, 1), 1) = mulHigh(-1, 1) = -1
1019+
*
1020+
* but
1021+
*
1022+
* mulHigh(-1, mulHigh(1, 1)) = mulHigh(-1, 0) = 0
1023+
*/
1024+
new BinaryOp.MulHigh(false, true) {
10161025

10171026
@Override
10181027
public Constant foldConstant(Constant const1, Constant const2) {
@@ -1085,7 +1094,16 @@ private long multiplyHigh(long x, long y, JavaKind javaKind) {
10851094
}
10861095
},
10871096

1088-
new BinaryOp.UMulHigh(true, true) {
1097+
/*
1098+
* UMulHigh is not associative, for example:
1099+
*
1100+
* uMulHigh(uMulHigh(-1L, Long.MAX_VALUE), 4L) = 1
1101+
*
1102+
* but
1103+
*
1104+
* uMulHigh(-1L, uMulHigh(Long.MAX_VALUE, 4L)) = 0
1105+
*/
1106+
new BinaryOp.UMulHigh(false, true) {
10891107

10901108
@Override
10911109
public Constant foldConstant(Constant const1, Constant const2) {

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -370,7 +370,7 @@ public static ValueNode reassociateUnmatchedValues(BinaryArithmeticNode<?> node,
370370
// Re-association from "Math.max(x, Math.max(y, C))" to "Math.max(Math.max(x, y), C)"
371371
return MaxNode.create(matchValue, MaxNode.create(otherValue1, otherValue2, view), view);
372372
} else {
373-
throw GraalError.shouldNotReachHere();
373+
throw GraalError.shouldNotReachHere("unhandled node in reassociation with constants: " + node);
374374
}
375375
}
376376

@@ -488,7 +488,7 @@ public static ValueNode reassociateMatchedValues(BinaryArithmeticNode<?> node, N
488488
return SignedFloatingIntegerRemNode.create(a, SignedFloatingIntegerRemNode.create(m1, m2, view, null, ((FloatingIntegerDivRemNode<?>) node).divisionOverflowIsJVMSCompliant()), view, null,
489489
((FloatingIntegerDivRemNode<?>) node).divisionOverflowIsJVMSCompliant());
490490
} else {
491-
throw GraalError.shouldNotReachHere();
491+
throw GraalError.shouldNotReachHere("unhandled node in reassociation with matched values: " + node);
492492
}
493493
}
494494

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ private static ValueNode canonical(IntegerMulHighNode self, ValueNode forY) {
8888
Constant c = forY.asConstant();
8989
if (c instanceof PrimitiveConstant && ((PrimitiveConstant) c).getJavaKind().isNumericInteger()) {
9090
long i = ((PrimitiveConstant) c).asLong();
91-
if (i == 0 || i == 1) {
91+
if (i == 0) {
9292
return ConstantNode.forIntegerStamp(self.stamp(NodeView.DEFAULT), 0);
9393
}
9494
}

‎compiler/src/org.graalvm.compiler.replacements.test/src/org/graalvm/compiler/replacements/test/MathMultiplyHighTest.java

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2020, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2020, 2022, Oracle and/or its affiliates. All rights reserved.
33
* Copyright (c) 2020, Arm Limited. All rights reserved.
44
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
55
*
@@ -25,7 +25,14 @@
2525
*/
2626
package org.graalvm.compiler.replacements.test;
2727

28+
import org.graalvm.compiler.nodes.ConstantNode;
29+
import org.graalvm.compiler.nodes.ValueNode;
2830
import org.graalvm.compiler.nodes.calc.IntegerMulHighNode;
31+
import org.graalvm.compiler.nodes.extended.OpaqueNode;
32+
import org.graalvm.compiler.nodes.spi.SimplifierTool;
33+
import org.graalvm.compiler.nodes.util.GraphUtil;
34+
35+
import org.junit.Assert;
2936
import org.junit.Test;
3037

3138
public class MathMultiplyHighTest extends MethodSubstitutionTest {
@@ -46,4 +53,24 @@ public void testMultiplyHigh() {
4653
}
4754
}
4855
}
56+
57+
private final SimplifierTool simplifierTool = GraphUtil.getDefaultSimplifier(getProviders(), false, null, getInitialOptions());
58+
59+
@Test
60+
public void testConstantFold() {
61+
IntegerMulHighNode mulHigh = new IntegerMulHighNode(ConstantNode.forLong(-1), ConstantNode.forLong(1));
62+
ValueNode canonical = mulHigh.canonical(simplifierTool);
63+
Assert.assertTrue("expected constant folding: " + canonical, canonical.isJavaConstant());
64+
long expected = Math.multiplyHigh(-1, 1);
65+
Assert.assertEquals("-1 *H 1", expected, canonical.asJavaConstant().asLong());
66+
}
67+
68+
@Test
69+
public void testSwapConstantAndMulHighZero() {
70+
IntegerMulHighNode mulHigh = new IntegerMulHighNode(ConstantNode.forLong(0), new OpaqueNode(ConstantNode.forLong(-1)));
71+
ValueNode canonical = mulHigh.canonical(simplifierTool);
72+
Assert.assertTrue("expected constant folding: " + canonical, canonical.isJavaConstant());
73+
long expected = Math.multiplyHigh(0, -1);
74+
Assert.assertEquals("0 *H -1", expected, canonical.asJavaConstant().asLong());
75+
}
4976
}

‎compiler/src/org.graalvm.compiler.truffle.test/src/org/graalvm/compiler/truffle/test/ExactMathTest.java

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -132,13 +132,19 @@ public void testLongMulHigh() {
132132
test("longMulHigh", Long.MIN_VALUE, 15L);
133133
test("longMulHigh", 15L, Long.MIN_VALUE);
134134
test("longMulHigh", Long.MIN_VALUE, Long.MIN_VALUE);
135+
test("longMulHigh1", Long.MIN_VALUE);
136+
test("longMulHighLeftAssociative", -1L, 1L, 1L);
137+
test("longMulHighRightAssociative", -1L, 1L, 1L);
135138
}
136139

137140
@Test
138141
public void testLongMulHighUnsigned() {
139142
test("longMulHighUnsigned", 7L, 15L);
140143
test("longMulHighUnsigned", Long.MAX_VALUE, 15L);
141144
test("longMulHighUnsigned", Long.MIN_VALUE, 15L);
145+
test("longMulHighUnsigned1", Long.MIN_VALUE);
146+
test("longMulHighUnsignedLeftAssociative", -1L, Long.MAX_VALUE, 4L);
147+
test("longMulHighUnsignedRightAssociative", -1L, Long.MAX_VALUE, 4L);
142148
}
143149

144150
@Test
@@ -233,10 +239,34 @@ public static long longMulHigh(long a, long b) {
233239
return ExactMath.multiplyHigh(a, b);
234240
}
235241

242+
public static long longMulHigh1(long a) {
243+
return ExactMath.multiplyHigh(a, 1L);
244+
}
245+
246+
public static long longMulHighLeftAssociative(long a, long b, long c) {
247+
return ExactMath.multiplyHigh(ExactMath.multiplyHigh(a, b), c);
248+
}
249+
250+
public static long longMulHighRightAssociative(long a, long b, long c) {
251+
return ExactMath.multiplyHigh(a, ExactMath.multiplyHigh(b, c));
252+
}
253+
236254
public static long longMulHighUnsigned(long a, long b) {
237255
return ExactMath.multiplyHighUnsigned(a, b);
238256
}
239257

258+
public static long longMulHighUnsigned1(long a) {
259+
return ExactMath.multiplyHighUnsigned(a, 1L);
260+
}
261+
262+
public static long longMulHighUnsignedLeftAssociative(long a, long b, long c) {
263+
return ExactMath.multiplyHighUnsigned(ExactMath.multiplyHighUnsigned(a, b), c);
264+
}
265+
266+
public static long longMulHighUnsignedRightAssociative(long a, long b, long c) {
267+
return ExactMath.multiplyHighUnsigned(a, ExactMath.multiplyHighUnsigned(b, c));
268+
}
269+
240270
public static float truncateFloat(float a) {
241271
return ExactMath.truncate(a);
242272
}

0 commit comments

Comments
 (0)