[GR-49215] DeoptProxyNode is not an ArrayLengthProvider.

PullRequest: graal/15701

Author: teshull

compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/util/GraphUtil.java

@@ -742,38 +742,53 @@ public static ValueNode arrayLength(ValueNode value, FindLengthMode mode, Consta
         return arrayLength(value, mode, constantReflection, null);
     }
 
+    /**
+     * Filters out non-constant results when requested.
+     */
+    private static ValueNode filterArrayLengthResult(ValueNode result, boolean allowOnlyConstantResult) {
+        return result == null || !allowOnlyConstantResult || result.isConstant() ? result : null;
+    }
+
     private static ValueNode arrayLength(ValueNode value, FindLengthMode mode, ConstantReflectionProvider constantReflection, EconomicMap<ValueNode, ValueNode> visitedPhiInputs) {
         Objects.requireNonNull(mode);
 
         EconomicMap<ValueNode, ValueNode> visitedPhiInputMap = visitedPhiInputs;
         ValueNode current = value;
         StructuredGraph graph = value.graph();
+        boolean allowOnlyConstantResult = false;
         do {
             CompilationAlarm.checkProgress(graph);
             /*
              * PiArrayNode implements ArrayLengthProvider and ValueProxy. We want to treat it as an
              * ArrayLengthProvider, therefore we check this case first.
              */
-            if (current instanceof ArrayLengthProvider) {
-                return ((ArrayLengthProvider) current).findLength(mode, constantReflection);
+            if (current instanceof ArrayLengthProvider provider) {
+                return filterArrayLengthResult(provider.findLength(mode, constantReflection), allowOnlyConstantResult);
 
-            } else if (current instanceof ValuePhiNode) {
+            } else if (current instanceof ValuePhiNode phi) {
                 if (visitedPhiInputMap == null) {
                     visitedPhiInputMap = EconomicMap.create();
                 }
-                return phiArrayLength((ValuePhiNode) current, mode, constantReflection, visitedPhiInputMap);
+                return filterArrayLengthResult(phiArrayLength(phi, mode, constantReflection, visitedPhiInputMap), allowOnlyConstantResult);
 
-            } else if (current instanceof ValueProxyNode) {
-                ValueProxyNode proxy = (ValueProxyNode) current;
+            } else if (current instanceof ValueProxyNode proxy) {
                 ValueNode length = arrayLength(proxy.getOriginalNode(), mode, constantReflection);
                 if (mode == ArrayLengthProvider.FindLengthMode.CANONICALIZE_READ && length != null && !length.isConstant()) {
                     length = new ValueProxyNode(length, proxy.proxyPoint());
                 }
-                return length;
+                return filterArrayLengthResult(length, allowOnlyConstantResult);
 
-            } else if (current instanceof ValueProxy) {
-                /* Written as a loop instead of a recursive call to reduce recursion depth. */
-                current = ((ValueProxy) current).getOriginalNode();
+            } else if (current instanceof LimitedValueProxy valueProxy) {
+                /*
+                 * Note is it usually recommended to check for ValueProxy, not LimitedValueProxy.
+                 * However, in this case we are intentionally unproxifying all LimitedValueProxies,
+                 * as we want constant lengths to be found across DeoptProxyNodes. When the result
+                 * is not a ValueProxy we limit the returned result to constant values.
+                 */
+                if (!(valueProxy instanceof ValueProxy)) {
+                    allowOnlyConstantResult = true;
+                }
+                current = valueProxy.getOriginalNode();
             } else {
                 return null;
             }

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/nodes/DeoptProxyNode.java

@@ -28,8 +28,6 @@
 import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.Node.ValueNumberable;
 import org.graalvm.compiler.graph.NodeClass;
-import org.graalvm.compiler.nodes.spi.Canonicalizable;
-import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.nodeinfo.InputType;
 import org.graalvm.compiler.nodeinfo.NodeCycles;
 import org.graalvm.compiler.nodeinfo.NodeInfo;
@@ -38,16 +36,14 @@
 import org.graalvm.compiler.nodes.ParameterNode;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.nodes.calc.FloatingNode;
-import org.graalvm.compiler.nodes.spi.ArrayLengthProvider;
+import org.graalvm.compiler.nodes.spi.Canonicalizable;
+import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
 import org.graalvm.compiler.nodes.spi.LIRLowerable;
 import org.graalvm.compiler.nodes.spi.LimitedValueProxy;
 import org.graalvm.compiler.nodes.spi.NodeLIRBuilderTool;
-import org.graalvm.compiler.nodes.util.GraphUtil;
 
 import com.oracle.svm.core.deopt.Deoptimizer;
 
-import jdk.vm.ci.meta.ConstantReflectionProvider;
-
 /**
  * Wraps locals and bytecode stack elements at deoptimization points. DeoptProxyNodes are inserted
  * in deoptimization target methods to avoid global value numbering and rescheduling of local
@@ -56,9 +52,14 @@
  * This is needed to ensure that the values, which are set by the {@link Deoptimizer} at the
  * deoptimization point, are really read from their locations (and not held in a temporary register,
  * etc.)
+ *
+ * Note the {@link #value} of the DeoptProxyNode may be another DeoptProxyNode (i.e., there may be a
+ * chain of DeoptProxyNodes leading to the original value). This is by design: linking to the
+ * preceding DeoptProxyNode allows a given DeoptEntry and its corresponding DeoptProxyNodes to be
+ * easily removed without causing correctness issues.
  */
 @NodeInfo(cycles = NodeCycles.CYCLES_0, size = NodeSize.SIZE_0)
-public final class DeoptProxyNode extends FloatingNode implements LimitedValueProxy, ValueNumberable, LIRLowerable, Canonicalizable, IterableNodeType, ArrayLengthProvider {
+public final class DeoptProxyNode extends FloatingNode implements LimitedValueProxy, ValueNumberable, LIRLowerable, Canonicalizable, IterableNodeType {
     public static final NodeClass<DeoptProxyNode> TYPE = NodeClass.create(DeoptProxyNode.class);
 
     /**
@@ -127,10 +128,4 @@ public boolean hasProxyPoint() {
     public ValueNode getProxyPoint() {
         return proxyPoint;
     }
-
-    @Override
-    public ValueNode findLength(FindLengthMode mode, ConstantReflectionProvider constantReflection) {
-        ValueNode length = GraphUtil.arrayLength(value, mode, constantReflection);
-        return length != null && length.isConstant() ? length : null;
-    }
 }