Use the equals method to compare speculations.

Author: pecimuth

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/CheckGraalInvariants.java

@@ -116,6 +116,7 @@
 import jdk.vm.ci.meta.ResolvedJavaField;
 import jdk.vm.ci.meta.ResolvedJavaMethod;
 import jdk.vm.ci.meta.ResolvedJavaType;
+import jdk.vm.ci.meta.SpeculationLog;
 import jdk.vm.ci.meta.Value;
 
 /**
@@ -324,6 +325,7 @@ public static void runTest(InvariantsTool tool) {
         verifiers.add(new VerifyUsageWithEquals(LIRKind.class));
         verifiers.add(new VerifyUsageWithEquals(ArithmeticOpTable.class));
         verifiers.add(new VerifyUsageWithEquals(ArithmeticOpTable.Op.class));
+        verifiers.add(new VerifyUsageWithEquals(SpeculationLog.Speculation.class, SpeculationLog.NO_SPECULATION));
 
         verifiers.add(new VerifySharedConstantEmptyArray());
         verifiers.add(new VerifyDebugUsage());

compiler/src/jdk.graal.compiler.test/src/jdk/graal/compiler/core/test/VerifyUsageWithEquals.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2013, 2019, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2013, 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
@@ -24,6 +24,9 @@
  */
 package jdk.graal.compiler.core.test;
 
+import java.util.Objects;
+
+import jdk.graal.compiler.api.replacements.SnippetReflectionProvider;
 import jdk.graal.compiler.core.common.type.ObjectStamp;
 import jdk.graal.compiler.nodes.Invoke;
 import jdk.graal.compiler.nodes.NodeView;
@@ -36,7 +39,8 @@
 import jdk.graal.compiler.nodes.spi.UncheckedInterfaceProvider;
 import jdk.graal.compiler.nodes.type.StampTool;
 import jdk.graal.compiler.phases.VerifyPhase;
-
+import jdk.vm.ci.meta.ConstantReflectionProvider;
+import jdk.vm.ci.meta.JavaConstant;
 import jdk.vm.ci.meta.JavaField;
 import jdk.vm.ci.meta.JavaKind;
 import jdk.vm.ci.meta.JavaMethod;
@@ -49,7 +53,8 @@
 /**
  * For certain types, object identity should not be used for object equality check. This phase
  * checks the correct usage of the given type. Equality checks with == or != (except null checks)
- * results in an {@link AssertionError}.
+ * results in an {@link AssertionError}. Optionally, a singleton value with which == and != checks
+ * are allowed as an exception may be provided.
  */
 public class VerifyUsageWithEquals extends VerifyPhase<CoreProviders> {
 
@@ -63,9 +68,34 @@ public boolean checkContract() {
      */
     private final Class<?> restrictedClass;
 
+    /**
+     * The value besides {@code null} for which equality checks with == or != are allowed.
+     */
+    private final Object safeSingletonValue;
+
     public VerifyUsageWithEquals(Class<?> restrictedClass) {
+        checkRestrictedClass(restrictedClass);
+        this.restrictedClass = restrictedClass;
+        this.safeSingletonValue = null;
+    }
+
+    /**
+     * Constructs a verifier to check that object identity is not used for equality checks except
+     * with {@code null} and the given singleton value.
+     *
+     * @param restrictedClass the class for which equality checks are restricted
+     * @param singletonValue the non-null value for which equality checks with == or != are allowed
+     *            as an exception
+     */
+    public <T> VerifyUsageWithEquals(Class<T> restrictedClass, T singletonValue) {
+        checkRestrictedClass(restrictedClass);
         this.restrictedClass = restrictedClass;
-        assert !restrictedClass.isInterface() || isTrustedInterface(restrictedClass);
+        Objects.requireNonNull(singletonValue);
+        this.safeSingletonValue = singletonValue;
+    }
+
+    private static void checkRestrictedClass(Class<?> restrictedClass) {
+        assert !restrictedClass.isInterface() || isTrustedInterface(restrictedClass) : "the restricted class must not be an untrusted interface";
     }
 
     private static final Class<?>[] trustedInterfaceTypes = {JavaType.class, JavaField.class, JavaMethod.class};
@@ -104,10 +134,28 @@ private boolean isAssignableToRestrictedType(ValueNode node, MetaAccessProvider
         return false;
     }
 
+    private boolean isSafeConstant(ValueNode node, ConstantReflectionProvider constantReflection, SnippetReflectionProvider snippetReflection) {
+        return isNullConstant(node) || isSafeSingletonValue(node, constantReflection, snippetReflection);
+    }
+
     private static boolean isNullConstant(ValueNode node) {
         return node.isConstant() && node.isNullConstant();
     }
 
+    private boolean isSafeSingletonValue(ValueNode node, ConstantReflectionProvider constantReflection, SnippetReflectionProvider snippetReflection) {
+        if (safeSingletonValue == null) {
+            return false;
+        }
+        JavaConstant javaConstant = node.asJavaConstant();
+        if (node instanceof LoadFieldNode loadField && loadField.isStatic() && loadField.field().isFinal()) {
+            javaConstant = constantReflection.readFieldValue(loadField.field(), null);
+        }
+        if (javaConstant == null) {
+            return false;
+        }
+        return safeSingletonValue == snippetReflection.asObject(Object.class, javaConstant);
+    }
+
     private static boolean isEqualsMethod(ResolvedJavaMethod method) {
         if (method.getName().equals("equals")) {
             Signature sig = method.getSignature();
@@ -129,12 +177,14 @@ private static boolean isThisParameter(ValueNode node) {
     }
 
     /**
-     * Checks whether the type of {@code x} is assignable to the restricted type and that {@code y}
-     * is not a null constant.
+     * Checks whether the type of {@code x} or {@code y} is assignable to the restricted type and
+     * that {@code x} and {@code y} are not safe constants.
      */
-    private boolean isIllegalUsage(ResolvedJavaMethod method, ValueNode x, ValueNode y, MetaAccessProvider metaAccess) {
-        if (isAssignableToRestrictedType(x, metaAccess) && !isNullConstant(y)) {
-            if (isEqualsMethod(method) && isThisParameter(x) || isThisParameter(y)) {
+    private boolean isIllegalUsage(ResolvedJavaMethod method, ValueNode x, ValueNode y, CoreProviders context) {
+        if ((isAssignableToRestrictedType(x, context.getMetaAccess()) || isAssignableToRestrictedType(y, context.getMetaAccess())) &&
+                        !isSafeConstant(x, context.getConstantReflection(), context.getSnippetReflection()) &&
+                        !isSafeConstant(y, context.getConstantReflection(), context.getSnippetReflection())) {
+            if (isEqualsMethod(method) && (isThisParameter(x) || isThisParameter(y))) {
                 return false;
             }
             return true;
@@ -151,7 +201,7 @@ protected void verify(StructuredGraph graph, CoreProviders context) {
 
             if (restrictedType.isAssignableFrom(method.getDeclaringClass())) {
                 // Allow violation in methods of the restricted type itself and its subclasses.
-            } else if (isIllegalUsage(method, cn.getX(), cn.getY(), context.getMetaAccess()) || isIllegalUsage(method, cn.getY(), cn.getX(), context.getMetaAccess())) {
+            } else if (isIllegalUsage(method, cn.getX(), cn.getY(), context)) {
                 throw new VerificationError("Verification of " + restrictedClass.getName() + " usage failed: Comparing " + cn.getX() + " and " + cn.getY() + " in " + method +
                                 " must use .equals() for object equality, not '==' or '!='");
             }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/ConditionalEliminationPhase.java

@@ -29,6 +29,7 @@
 import java.util.ArrayDeque;
 import java.util.Deque;
 import java.util.List;
+import java.util.Objects;
 
 import org.graalvm.collections.EconomicMap;
 import org.graalvm.collections.EconomicSet;
@@ -355,7 +356,7 @@ public HIRBlock enter(HIRBlock b) {
                                     Speculation speculation = trueGuard.getSpeculation();
                                     if (speculation == null) {
                                         speculation = falseGuard.getSpeculation();
-                                    } else if (falseGuard.getSpeculation() != null && falseGuard.getSpeculation() != speculation) {
+                                    } else if (falseGuard.getSpeculation() != null && !falseGuard.getSpeculation().equals(speculation)) {
                                         // Cannot optimize due to different speculations.
                                         continue;
                                     }
@@ -1107,7 +1108,7 @@ private boolean canScheduleAbove(Node n, Node target, ValueNode knownToBeAbove)
 
         protected boolean foldGuard(DeoptimizingGuard thisGuard, DeoptimizingGuard otherGuard, boolean outcome, Stamp guardedValueStamp, ConditionalEliminationUtil.GuardRewirer rewireGuardFunction) {
             DeoptimizationAction action = mergeActions(otherGuard.getAction(), thisGuard.getAction());
-            if (action != null && otherGuard.getSpeculation() == thisGuard.getSpeculation()) {
+            if (action != null && Objects.equals(otherGuard.getSpeculation(), thisGuard.getSpeculation())) {
                 LogicNode condition = (LogicNode) thisGuard.getCondition().copyWithInputs();
                 /*
                  * We have ...; guard(C1); guard(C2);...