[GR-59503] Stricter checks for CFG and schedule validity

PullRequest: graal/19225

Author: gergo-

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/cfg/ControlFlowGraph.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 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
@@ -55,6 +55,7 @@
 import jdk.graal.compiler.nodes.DeoptimizeNode;
 import jdk.graal.compiler.nodes.FixedNode;
 import jdk.graal.compiler.nodes.FixedWithNextNode;
+import jdk.graal.compiler.nodes.GraphState;
 import jdk.graal.compiler.nodes.LoopBeginNode;
 import jdk.graal.compiler.nodes.LoopEndNode;
 import jdk.graal.compiler.nodes.LoopExitNode;
@@ -333,6 +334,14 @@ private static ControlFlowGraph lookupCached(StructuredGraph graph, boolean modi
                 return lastCFG;
             }
         }
+        if (graph.getLastSchedule() != null && !graph.isAfterStage(GraphState.StageFlag.FINAL_SCHEDULE)) {
+            /*
+             * There is no up to date CFG. If there is a current schedule, its CFG must also be out
+             * of date. So invalidate the schedule. The only exception is during cleanup phases
+             * after final scheduling, where we don't want to destroy that schedule.
+             */
+            graph.clearLastSchedule();
+        }
         return null;
     }
 
@@ -398,7 +407,7 @@ public EconomicMap<LoopBeginNode, LoopFrequencyData> getLocalLoopFrequencyData()
      * will return the updated value. This is useful for phases to record temporary effects of
      * transformations on loop frequencies, without having to recompute a CFG.
      * </p>
-     *
+     * <p>
      * The updated frequency is a cached value local to this CFG. It is <em>not</em> persisted in
      * the IR graph. Newly computed {@link ControlFlowGraph} instances will recompute a frequency
      * from loop exit probabilities, they will not see this locally cached value. Persistent changes
@@ -688,7 +697,7 @@ private boolean verifyRPOInnerLoopsFirst() {
      * it is not an endless loop) {@link LoopExitNode}. For every path exiting a loop a
      * {@link LoopExitNode} is required. There is one exception to that rule:
      * {@link DeoptimizeNode}.
-     *
+     * <p>
      * Graal does not mandate that a {@link DeoptimizeNode} is preceded by a {@link LoopExitNode}.
      * In the following example
      *
@@ -699,7 +708,7 @@ private boolean verifyRPOInnerLoopsFirst() {
      *     }
      * }
      * </pre>
-     *
+     * <p>
      * the IR does not have a preceding loop exit node before the deopt node. However, for regular
      * control flow sinks (returns, throws, etc) like in the following example
      *
@@ -710,9 +719,9 @@ private boolean verifyRPOInnerLoopsFirst() {
      *     }
      * }
      * </pre>
-     *
+     * <p>
      * Graal IR creates a {@link LoopExitNode} before the {@link ReturnNode}.
-     *
+     * <p>
      * Because of the "imprecision" in the definition a regular basic block exiting a loop and a
      * "dominator tree" loop exit are not necessarily the same. If a path after a control flow split
      * unconditionally flows into a deopt it is a "dominator loop exit" while a regular loop exit
@@ -810,9 +819,9 @@ private boolean rpoInnerLoopsFirst(Consumer<HIRBlock> perBasicBlockOption, Consu
     /**
      * Determine if sequential predecessor blocks of this block in a not-fully-canonicalized graph
      * exit a loop.
-     *
+     * <p>
      * Example: Sequential basic block: loop exit -> invoke -> killing begin -> loopend/exit
-     *
+     * <p>
      * These cases cause problems in the {@link #verifyRPOInnerLoopsFirst()} loop verification of
      * inner loop blocks because the granularity of loop ends and exits are not on block boundaries:
      * a loop exit block can also be a loop end to an outer loop, which makes verification that the

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopsData.java

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012, 2021, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2012, 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
@@ -27,17 +27,19 @@
 import java.util.ArrayList;
 import java.util.List;
 
-import jdk.graal.compiler.nodes.LoopBeginNode;
-import jdk.graal.compiler.nodes.StructuredGraph;
-import jdk.graal.compiler.nodes.ValueNode;
-import jdk.graal.compiler.nodes.cfg.ControlFlowGraph;
-import jdk.graal.compiler.nodes.cfg.HIRBlock;
 import org.graalvm.collections.EconomicMap;
 import org.graalvm.collections.EconomicSet;
 import org.graalvm.collections.Equivalence;
+
 import jdk.graal.compiler.core.common.cfg.CFGLoop;
 import jdk.graal.compiler.core.common.util.ReversedList;
 import jdk.graal.compiler.debug.DebugContext;
+import jdk.graal.compiler.nodes.GraphState;
+import jdk.graal.compiler.nodes.LoopBeginNode;
+import jdk.graal.compiler.nodes.StructuredGraph;
+import jdk.graal.compiler.nodes.ValueNode;
+import jdk.graal.compiler.nodes.cfg.ControlFlowGraph;
+import jdk.graal.compiler.nodes.cfg.HIRBlock;
 
 /**
  * A data structure representing all the information about all loops in the given graph. Data about
@@ -86,7 +88,9 @@ protected LoopsData(final StructuredGraph graph, ControlFlowGraph preComputedCFG
         DebugContext debug = graph.getDebug();
         if (preComputedCFG == null) {
             try (DebugContext.Scope s = debug.scope("ControlFlowGraph")) {
-                this.cfg = ControlFlowGraph.newBuilder(graph).connectBlocks(true).computeLoops(true).computeDominators(true).computePostdominators(true).computeFrequency(true).build();
+                boolean backendBlocks = graph.isAfterStage(GraphState.StageFlag.FINAL_SCHEDULE);
+                this.cfg = ControlFlowGraph.newBuilder(graph).connectBlocks(true).backendBlocks(backendBlocks).computeLoops(true).computeDominators(true).computePostdominators(true).computeFrequency(
+                                true).build();
             } catch (Throwable e) {
                 throw debug.handle(e);
             }

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

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2015, 2023, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2015, 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
@@ -195,7 +195,12 @@ protected void run(StructuredGraph graph, CoreProviders context) {
                     cfg.visitDominatorTree(new MoveGuardsUpwards(), deferLoopExits);
                 }
                 try (DebugContext.Scope scheduleScope = graph.getDebug().scope(SchedulePhase.class)) {
+                    if (!graph.isLastCFGValid()) {
+                        cfg = null;
+                    }
                     SchedulePhase.run(graph, SchedulePhase.SchedulingStrategy.EARLIEST_WITH_GUARD_ORDER, cfg, context, false);
+                    cfg = graph.getLastCFG();
+                    cfg.computePostdominators();
                 } catch (Throwable t) {
                     throw graph.getDebug().handle(t);
                 }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/schedule/SchedulePhase.java

@@ -266,6 +266,10 @@ public Instance(ControlFlowGraph cfg, boolean supportsImplicitNullChecks) {
         public void run(StructuredGraph graph, SchedulingStrategy selectedStrategy, boolean immutableGraph) {
             if (this.cfg == null) {
                 this.cfg = ControlFlowGraph.computeForSchedule(graph);
+            } else {
+                GraalError.guarantee(this.cfg == graph.getLastCFG() && graph.isLastCFGValid(),
+                                "Cannot compute schedule for stale CFG; given: %s, graph's last CFG is %s, is valid: %s.",
+                                this.cfg, graph.getLastCFG(), graph.isLastCFGValid());
             }
 
             NodeMap<HIRBlock> currentNodeMap = graph.createNodeMap();