[GR-40383] Infer non-null stamp for mutually recursive non-null phis

PullRequest: graal/12466

Author: gergo-

compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/NodeFlood.java

@@ -28,6 +28,31 @@
 import java.util.Iterator;
 import java.util.Queue;
 
+import org.graalvm.compiler.graph.NodeWorkList.SingletonNodeWorkList;
+
+/**
+ * A data structure for visiting nodes in a graph iteratively. Each node added to the flood will be
+ * visited exactly once. New nodes to visit can be added during the iteration.
+ * </p>
+ *
+ * A typical use of this class looks like:
+ *
+ * <pre>
+ * NodeFlood flood = graph.createNodeFlood();
+ * flood.add(initialNodeOfInterest);
+ * for (Node n : flood) {
+ *     processNode(n);
+ *     if (inputsAreOfInterest(n)) {
+ *         flood.addAll(n.inputs());
+ *     }
+ * }
+ * </pre>
+ *
+ * This class is equivalent to {@link SingletonNodeWorkList}, except that
+ * {@link SingletonNodeWorkList} ignores deleted nodes both when adding and enumerating nodes. In
+ * contrast, it is an error to try to add deleted nodes to a {@link NodeFlood}, but
+ * {@link NodeFlood} will enumerate nodes that are deleted while they are enqueued for enumeration.
+ */
 public final class NodeFlood implements Iterable<Node> {
 
     private final NodeBitMap visited;

compiler/src/org.graalvm.compiler.graph/src/org/graalvm/compiler/graph/NodeWorkList.java

@@ -73,6 +73,36 @@ public void remove() {
         }
     }
 
+    /**
+     * A data structure for visiting nodes in a graph iteratively. Each node added to the worklist
+     * can be visited multiple times. New nodes to visit can be added during the iteration.
+     * </p>
+     *
+     * Nodes are only re-added to the worklist if they are not already enqueued for a future visit.
+     * Therefore calling {@link #add} with a node ensures that that node will be visited in the
+     * future, but the total number of visits may be less than the number of {@link #add} calls.
+     * </p>
+     *
+     * Iteration stops after enumeration of {@code iterationLimitPerNode * graph.getNodeCount()}
+     * nodes. The limit is thus an average limit over all nodes, and individual nodes may be visited
+     * more than {@code iterationLimitPerNode} times.
+     * </p>
+     *
+     * A typical use of this class looks like:
+     *
+     * <pre>
+     * NodeWorkList worklist = graph.createIterativeNodeWorkList(fill, iterationLimitPerNode);
+     * worklist.add(initialNodeOfInterest);
+     * for (Node n : worklist) {
+     *     processNode(n);
+     *     if (inputsAreOfInterest(n)) {
+     *         worklist.addAll(n.inputs());
+     *     }
+     * }
+     * </pre>
+     *
+     * Deleted nodes are ignored both when adding and when enumerating nodes.
+     */
     public static final class IterativeNodeWorkList extends NodeWorkList {
         private static final int EXPLICIT_BITMAP_THRESHOLD = 10;
         protected NodeBitMap inQueue;
@@ -195,6 +225,29 @@ private void inflateToBitMap(Graph graph) {
         }
     }
 
+    /**
+     * A data structure for visiting nodes in a graph iteratively. Each node added to the worklist
+     * will be visited exactly once. New nodes to visit can be added during the iteration.
+     * </p>
+     *
+     * A typical use of this class looks like:
+     *
+     * <pre>
+     * NodeWorkList worklist = graph.createNodeWorkList();
+     * worklist.add(initialNodeOfInterest);
+     * for (Node n : worklist) {
+     *     processNode(n);
+     *     if (inputsAreOfInterest(n)) {
+     *         worklist.addAll(n.inputs());
+     *     }
+     * }
+     * </pre>
+     *
+     * This class is equivalent to {@link NodeFlood}, except that {@link SingletonNodeWorkList}
+     * ignores deleted nodes both when adding and enumerating nodes. In contrast, it is an error to
+     * try to add deleted nodes to a {@link NodeFlood}, but {@link NodeFlood} will enumerate nodes
+     * that are deleted while they are enqueued for enumeration.
+     */
     public static final class SingletonNodeWorkList extends NodeWorkList {
         protected final NodeBitMap visited;
 

compiler/src/org.graalvm.compiler.nodes/src/org/graalvm/compiler/nodes/ValuePhiNode.java

@@ -26,10 +26,13 @@
 
 import java.util.Map;
 
+import org.graalvm.compiler.core.common.type.AbstractPointerStamp;
 import org.graalvm.compiler.core.common.type.IntegerStamp;
 import org.graalvm.compiler.core.common.type.Stamp;
 import org.graalvm.compiler.core.common.type.StampFactory;
+import org.graalvm.compiler.graph.Node;
 import org.graalvm.compiler.graph.NodeClass;
+import org.graalvm.compiler.graph.NodeFlood;
 import org.graalvm.compiler.graph.NodeInputList;
 import org.graalvm.compiler.nodeinfo.NodeInfo;
 import org.graalvm.compiler.nodes.calc.AddNode;
@@ -80,9 +83,55 @@ public boolean inferStamp() {
         } else if (stamp.isCompatible(valuesStamp)) {
             valuesStamp = stamp.join(valuesStamp);
         }
+
+        Stamp maybeNonNullStamp = tryInferNonNullStamp(valuesStamp);
+        if (maybeNonNullStamp != valuesStamp) {
+            valuesStamp = maybeNonNullStamp;
+        }
+
         return updateStamp(valuesStamp);
     }
 
+    /**
+     * See if this phi has a pointer stamp and is part of a set of mutually recursive phis that only
+     * have each other or non-null values as inputs. In this case we can infer a non-null stamp.
+     *
+     * @return a non-null version of the original {@code valuesStamp} if it could be improved, the
+     *         unchanged {@code valuesStamp} otherwise
+     */
+    private Stamp tryInferNonNullStamp(Stamp valuesStamp) {
+        if (isAlive() && isLoopPhi() && valuesStamp instanceof AbstractPointerStamp) {
+            AbstractPointerStamp pointerStamp = (AbstractPointerStamp) valuesStamp;
+            if (!pointerStamp.alwaysNull() && !pointerStamp.nonNull() && StampTool.isPointerNonNull(firstValue())) {
+                // Fail early if this phi already has possibly null non-phi inputs.
+                for (ValueNode value : values()) {
+                    if (value == this || value instanceof ValuePhiNode) {
+                        continue;
+                    } else if (!StampTool.isPointerNonNull(value)) {
+                        return valuesStamp;
+                    }
+                }
+                // Check input phis recursively.
+                NodeFlood flood = new NodeFlood(graph());
+                flood.addAll(values().filter(ValuePhiNode.class));
+                for (Node node : flood) {
+                    if (node instanceof ValuePhiNode) {
+                        for (ValueNode value : ((ValuePhiNode) node).values()) {
+                            if (value == this || value instanceof ValuePhiNode) {
+                                flood.add(value);
+                            } else if (!StampTool.isPointerNonNull(value)) {
+                                return valuesStamp;
+                            }
+                        }
+                    }
+                }
+                // All transitive inputs are non-null.
+                return ((AbstractPointerStamp) valuesStamp).asNonNull();
+            }
+        }
+        return valuesStamp;
+    }
+
     @Override
     public boolean verify() {
         Stamp s = null;

compiler/src/org.graalvm.compiler.replacements.test/src/org/graalvm/compiler/replacements/test/MonitorTest.java

@@ -25,7 +25,7 @@
 package org.graalvm.compiler.replacements.test;
 
 import org.junit.Test;
-
+import org.graalvm.compiler.api.directives.GraalDirectives;
 import org.graalvm.compiler.core.test.GraalCompilerTest;
 import org.graalvm.compiler.nodes.ValueNode;
 import org.graalvm.compiler.replacements.BoxingSnippets;
@@ -232,4 +232,20 @@ public void test8() {
         test("lockBoxedLong", Long.MAX_VALUE - 1);
         test("lockBoxedLong", Long.MIN_VALUE + 1);
     }
+
+    static void loopPhi(boolean f) {
+        Object j = new Object();
+        int n = f ? 10 : 11;
+        for (int i = 0; GraalDirectives.injectIterationCount(11.5, i < n); i++) {
+            synchronized (j) {
+                j = f ? new Object() : j;
+            }
+        }
+    }
+
+    @Test
+    public void testLoopPhi() {
+        test("loopPhi", true);
+        test("loopPhi", false);
+    }
 }