[GR-43958] Fix stamp logic in CopySignNode.

PullRequest: graal/13686
guizmaii · Feb 4, 2023 · 00cf640 · 00cf640
2 parents 4a9636e + 564a6c6
commit 00cf640
Show file tree

Hide file tree

Showing 3 changed files with 56 additions and 23 deletions.
diff --git a/....graalvm.compiler.core.test/src/org/graalvm/compiler/core/test/MathCopySignStampTest.java b/....graalvm.compiler.core.test/src/org/graalvm/compiler/core/test/MathCopySignStampTest.java
@@ -90,6 +90,22 @@ public void testFloatCopySign() throws InvalidInstalledCodeException {
         }
     }
 
+    public static int badCopyStampGR43958(short magnitude) {
+        /*
+         * Previously the stamp of the copySign would be incorrect.
+         */
+        return (short) Math.copySign(magnitude, -942.5804f);
+    }
+
+    @Test
+    public void testGR43958() throws InvalidInstalledCodeException {
+        short[] shortValues = {42, -42, 95};
+        for (short magnitude : shortValues) {
+            InstalledCode code = getCode(getResolvedJavaMethod("badCopyStampGR43958"), null, true);
+            Assert.assertEquals(badCopyStampGR43958(magnitude), (int) code.executeVarargs(magnitude), 0);
+        }
+    }
+
     private static final double[] doubleValues = {
                     0.0d,
                     -0.0d,

diff --git a/...iler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/CopySignNode.java b/...iler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/CopySignNode.java
@@ -51,36 +51,52 @@ public CopySignNode(ValueNode magnitude, ValueNode sign) {
         super(TYPE, computeStamp(magnitude.stamp(NodeView.DEFAULT), sign.stamp(NodeView.DEFAULT)), magnitude, sign);
     }
 
-    public static Stamp computeStamp(Stamp stampX, Stamp stampY) {
-        FloatStamp floatStampX = (FloatStamp) stampX;
-        FloatStamp floatStampY = (FloatStamp) stampY;
-        if (floatStampX.isNaN()) {
-            return stampX;
+    public static Stamp computeStamp(Stamp magnitude, Stamp sign) {
+        FloatStamp magnitudeStamp = (FloatStamp) magnitude;
+        FloatStamp signStamp = (FloatStamp) sign;
+        if (magnitudeStamp.isNaN()) {
+            return magnitude;
         }
-        if (floatStampY.isNonNaN()) {
-            if (floatStampY.lowerBound() > 0) {
-                if (floatStampX.lowerBound() > 0) {
-                    return floatStampX;
+        if (signStamp.isNonNaN()) {
+            if (signStamp.lowerBound() > 0) {
+                // the end result will be non-negative
+                if (magnitudeStamp.lowerBound() > 0) {
+                    // We know the entire range is above 0: leave it unchanged.
+                    return magnitudeStamp;
                 }
-                if (floatStampX.upperBound() < 0) {
-                    return new FloatStamp(floatStampX.getBits(), -floatStampX.upperBound(), -floatStampX.lowerBound(), floatStampX.isNonNaN());
+                if (magnitudeStamp.upperBound() < 0) {
+                    // We know that the entire range is below 0
+                    // flip [lower, upper] to [-upper, -lower]
+                    return new FloatStamp(magnitudeStamp.getBits(), -magnitudeStamp.upperBound(), -magnitudeStamp.lowerBound(), magnitudeStamp.isNonNaN());
                 }
-                return new FloatStamp(floatStampX.getBits(), Math.min(-floatStampX.lowerBound(), floatStampX.upperBound()),
-                                Math.max(-floatStampX.lowerBound(), floatStampX.upperBound()), floatStampX.isNonNaN());
+                // We know lowerBound <= 0 and upperBound >= 0:
+                // the new range is [0, Math.max(-lower, upper)]
+                return new FloatStamp(magnitudeStamp.getBits(), 0,
+                                Math.max(-magnitudeStamp.lowerBound(), magnitudeStamp.upperBound()), magnitudeStamp.isNonNaN());
             }
-            if (floatStampY.upperBound() < 0) {
-                if (floatStampX.upperBound() < 0) {
-                    return floatStampX;
+            if (signStamp.upperBound() < 0) {
+                // the result will be non-positive
+                if (magnitudeStamp.upperBound() < 0) {
+                    // We know the entire range is below 0: leave it unchanged.
+                    return magnitudeStamp;
                 }
-                if (floatStampX.lowerBound() > 0) {
-                    return new FloatStamp(floatStampX.getBits(), -floatStampX.upperBound(), -floatStampX.lowerBound(), floatStampX.isNonNaN());
+                if (magnitudeStamp.lowerBound() > 0) {
+                    // We know that the entire range is above 0
+                    // flip [lower, upper] to [-upper,-lower]
+                    return new FloatStamp(magnitudeStamp.getBits(), -magnitudeStamp.upperBound(), -magnitudeStamp.lowerBound(), magnitudeStamp.isNonNaN());
                 }
-                return new FloatStamp(floatStampX.getBits(), Math.min(floatStampX.lowerBound(), -floatStampX.upperBound()),
-                                Math.max(floatStampX.lowerBound(), -floatStampX.upperBound()), floatStampX.isNonNaN());
+                // We know lowerBound <= 0 and upperBound >= 0
+                // the new range is [Math.min(lower, -upper), 0]
+                return new FloatStamp(magnitudeStamp.getBits(), Math.min(magnitudeStamp.lowerBound(), -magnitudeStamp.upperBound()),
+                                0, magnitudeStamp.isNonNaN());
             }
         }
-        return new FloatStamp(floatStampX.getBits(), Math.min(floatStampX.lowerBound(), -floatStampX.upperBound()), Math.max(-floatStampX.lowerBound(), floatStampX.upperBound()),
-                        floatStampX.isNonNaN());
+        /*
+         * We have no information on whether the range will be flipped or not. Hence, we have to
+         * expand the result to be the union of [lower, upper] and [-upper, -lower].
+         */
+        return new FloatStamp(magnitudeStamp.getBits(), Math.min(magnitudeStamp.lowerBound(), -magnitudeStamp.upperBound()), Math.max(-magnitudeStamp.lowerBound(), magnitudeStamp.upperBound()),
+                        magnitudeStamp.isNonNaN());
     }
 
     @Override

diff --git a/compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/SignumNode.java b/compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/calc/SignumNode.java
@@ -67,7 +67,8 @@ private static Stamp computeStamp(Stamp stamp) {
                 return new FloatStamp(floatStamp.getBits(), -1.0D, -1.0D, true);
             }
         }
-        FloatStamp result = new FloatStamp(floatStamp.getBits(), Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, !floatStamp.canBeNaN());
+        // Initially make an empty stamp.
+        FloatStamp result = new FloatStamp(floatStamp.getBits(), Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, floatStamp.isNonNaN());
         if (floatStamp.contains(0.0d)) {
             // this also covers stamp.contains(-0.0d)
             result = (FloatStamp) result.meet(new FloatStamp(floatStamp.getBits(), 0.0d, 0.0d, true));