[GR-51800] Backport to 24.0: Don't optimize away Narrow in comparison…

…s with mixed signedness PullRequest: graal/16965
oracle · Feb 16, 2024 · 1af04a5 · 1af04a5
2 parents e5541aa + a0e9619
commit 1af04a5
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 10 deletions.
diff --git a/...c/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/NarrowPreservesOrderTest.java b/...c/jdk.graal.compiler.test/src/jdk/graal/compiler/nodes/test/NarrowPreservesOrderTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2022, 2024, 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
@@ -29,6 +29,8 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 
+import org.junit.Test;
+
 import jdk.graal.compiler.core.common.NumUtil;
 import jdk.graal.compiler.core.common.calc.CanonicalCondition;
 import jdk.graal.compiler.core.common.type.IntegerStamp;
@@ -38,8 +40,6 @@
 import jdk.graal.compiler.graph.test.GraphTest;
 import jdk.graal.compiler.nodes.ParameterNode;
 import jdk.graal.compiler.nodes.calc.NarrowNode;
-import org.junit.Test;
-
 import jdk.vm.ci.code.CodeUtil;
 import jdk.vm.ci.meta.JavaKind;
 
@@ -97,7 +97,7 @@ public void testByte() {
         testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(8)), 8, LT, false);
         testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(8)), 8, BT, true);
         testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Byte), 32), 8, LT, true);
-        testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Byte), 32), 8, BT, true);
+        testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Byte), 32), 8, BT, false);
         testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(8), 32), 8, LT, false);
         testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(8), 32), 8, BT, true);
 
@@ -114,7 +114,7 @@ public void testShort() {
         testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(16)), 16, LT, false);
         testPreserveOrder(forConstantInt(NumUtil.maxValueUnsigned(16)), 16, BT, true);
         testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Short), 32), 16, LT, true);
-        testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Short), 32), 16, BT, true);
+        testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Short), 32), 16, BT, false);
         testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(16), 32), 16, LT, false);
         testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(16), 32), 16, BT, true);
 
@@ -131,7 +131,7 @@ public void testInt() {
         testPreserveOrder(forConstantLong(NumUtil.maxValueUnsigned(32)), 32, LT, false);
         testPreserveOrder(forConstantLong(NumUtil.maxValueUnsigned(32)), 32, BT, true);
         testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Int), 64), 32, LT, true);
-        testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Int), 64), 32, BT, true);
+        testPreserveOrder(signExtend(StampFactory.forKind(JavaKind.Int), 64), 32, BT, false);
         testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(32), 64), 32, LT, false);
         testPreserveOrder(zeroExtend(StampFactory.forUnsignedInteger(32), 64), 32, BT, true);
 

diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/calc/NarrowNode.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/calc/NarrowNode.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2014, 2024, 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
@@ -42,7 +42,6 @@
 import jdk.graal.compiler.nodes.ValueNode;
 import jdk.graal.compiler.nodes.spi.CanonicalizerTool;
 import jdk.graal.compiler.nodes.spi.NodeLIRBuilderTool;
-
 import jdk.vm.ci.code.CodeUtil;
 
 /**
@@ -129,10 +128,22 @@ public boolean preservesOrder(CanonicalCondition cond) {
         switch (cond) {
             case LT:
                 return isSignedLossless();
+            /*
+             * We may use signed stamps to represent unsigned integers. This narrow preserves order
+             * if it is signed lossless and is being compared to another narrow that is signed
+             * lossless, or if it is unsigned lossless and the other narrow is also unsigned
+             * lossless. We don't have access to the other narrow here, so we must make a
+             * conservative choice. We can rely on the fact that the same computation will be
+             * performed on the other narrow, so it will make the same choice.
+             *
+             * Most Java values are signed, so we expect the signed case to be more relevant for
+             * equals comparison. In contrast, the unsigned case should be more relevant for
+             * unsigned less than comparisons.
+             */
             case EQ:
+                return isSignedLossless();
             case BT:
-                // We may use signed stamps to represent unsigned integers.
-                return isSignedLossless() || isUnsignedLossless();
+                return isUnsignedLossless();
             default:
                 throw GraalError.shouldNotReachHere("Unsupported canonical condition."); // ExcludeFromJacocoGeneratedReport
         }