[GR-8428] LoopFragment detection potentially processes floating nodes…

… multiple times.

PullRequest: graal/1078

compiler/src/org.graalvm.compiler.loop/src/org/graalvm/compiler/loop/LoopFragment.java

@@ -35,6 +35,7 @@
 import org.graalvm.compiler.graph.NodeBitMap;
 import org.graalvm.compiler.graph.iterators.NodeIterable;
 import org.graalvm.compiler.nodes.AbstractBeginNode;
+import org.graalvm.compiler.nodes.AbstractMergeNode;
 import org.graalvm.compiler.nodes.EndNode;
 import org.graalvm.compiler.nodes.FixedNode;
 import org.graalvm.compiler.nodes.FrameState;
@@ -208,6 +209,12 @@ protected static void computeNodes(NodeBitMap nodes, Graph graph, Iterable<Abstr
                     NodeWithState withState = (NodeWithState) n;
                     withState.states().forEach(state -> state.applyToVirtual(node -> nodes.mark(node)));
                 }
+                if (n instanceof AbstractMergeNode) {
+                    // if a merge is in the loop, all of its phis are also in the loop
+                    for (PhiNode phi : ((AbstractMergeNode) n).phis()) {
+                        nodes.mark(phi);
+                    }
+                }
                 nodes.mark(n);
             }
         }
@@ -246,6 +253,17 @@ protected static void computeNodes(NodeBitMap nodes, Graph graph, Iterable<Abstr
                 if (n instanceof MonitorEnterNode) {
                     markFloating(worklist, ((MonitorEnterNode) n).getMonitorId(), nodes, nonLoopNodes);
                 }
+                if (n instanceof AbstractMergeNode) {
+                    /*
+                     * Since we already marked all phi nodes as being in the loop to break cycles,
+                     * we also have to iterate over their usages here.
+                     */
+                    for (PhiNode phi : ((AbstractMergeNode) n).phis()) {
+                        for (Node usage : phi.usages()) {
+                            markFloating(worklist, usage, nodes, nonLoopNodes);
+                        }
+                    }
+                }
                 for (Node usage : n.usages()) {
                     markFloating(worklist, usage, nodes, nonLoopNodes);
                 }
@@ -263,6 +281,20 @@ static class WorkListEntry {
             this.usages = n.usages().iterator();
             this.isLoopNode = loopNodes.isMarked(n);
         }
+
+        @Override
+        public boolean equals(Object obj) {
+            if (!(obj instanceof WorkListEntry)) {
+                return false;
+            }
+            WorkListEntry other = (WorkListEntry) obj;
+            return this.n == other.n;
+        }
+
+        @Override
+        public int hashCode() {
+            return n.hashCode();
+        }
     }
 
     static TriState isLoopNode(Node n, NodeBitMap loopNodes, NodeBitMap nonLoopNodes) {
@@ -272,32 +304,24 @@ static TriState isLoopNode(Node n, NodeBitMap loopNodes, NodeBitMap nonLoopNodes
         if (nonLoopNodes.isMarked(n)) {
             return TriState.FALSE;
         }
-        if (n instanceof FixedNode) {
+        if (n instanceof FixedNode || n instanceof PhiNode) {
+            // phi nodes are treated the same as fixed nodes in this algorithm to break cycles
             return TriState.FALSE;
         }
-        boolean mark = false;
-        if (n instanceof PhiNode) {
-            PhiNode phi = (PhiNode) n;
-            mark = loopNodes.isMarked(phi.merge());
-            if (mark) {
-                /*
-                 * This Phi is a loop node but the inputs might not be so they must be processed by
-                 * the caller.
-                 */
-                loopNodes.mark(n);
-            } else {
-                nonLoopNodes.mark(n);
-                return TriState.FALSE;
-            }
-        }
         return TriState.UNKNOWN;
     }
 
+    private static void pushWorkList(Deque<WorkListEntry> workList, Node node, NodeBitMap loopNodes) {
+        WorkListEntry entry = new WorkListEntry(node, loopNodes);
+        assert !workList.contains(entry) : "node " + node + " added to worklist twice";
+        workList.push(entry);
+    }
+
     private static void markFloating(Deque<WorkListEntry> workList, Node start, NodeBitMap loopNodes, NodeBitMap nonLoopNodes) {
         if (isLoopNode(start, loopNodes, nonLoopNodes).isKnown()) {
             return;
         }
-        workList.push(new WorkListEntry(start, loopNodes));
+        pushWorkList(workList, start, loopNodes);
         while (!workList.isEmpty()) {
             WorkListEntry currentEntry = workList.peek();
             if (currentEntry.usages.hasNext()) {
@@ -308,7 +332,7 @@ private static void markFloating(Deque<WorkListEntry> workList, Node start, Node
                         currentEntry.isLoopNode = true;
                     }
                 } else {
-                    workList.push(new WorkListEntry(current, loopNodes));
+                    pushWorkList(workList, current, loopNodes);
                 }
             } else {
                 workList.pop();