From 3d1f83ae69ea051d59f70403d55fcbdfb0718ed5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C3=B6=20Barany?= Date: Tue, 5 Nov 2024 13:58:01 +0100 Subject: [PATCH] Stricter checks for CFG and schedule validity --- .../compiler/nodes/cfg/ControlFlowGraph.java | 25 +++++++++++++------ .../graal/compiler/nodes/loop/LoopsData.java | 18 +++++++------ .../common/ConditionalEliminationPhase.java | 7 +++++- .../phases/schedule/SchedulePhase.java | 4 +++ 4 files changed, 38 insertions(+), 16 deletions(-) diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/cfg/ControlFlowGraph.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/cfg/ControlFlowGraph.java index 779c7a5b3cf5..c297e9ca5b2e 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/cfg/ControlFlowGraph.java +++ b/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 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. *

- * + *

* The updated frequency is a cached value local to this CFG. It is not 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}. - * + *

* 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() { * } * } * - * + *

* 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() { * } * } * - * + *

* Graal IR creates a {@link LoopExitNode} before the {@link ReturnNode}. - * + *

* 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 perBasicBlockOption, Consu /** * Determine if sequential predecessor blocks of this block in a not-fully-canonicalized graph * exit a loop. - * + *

* Example: Sequential basic block: loop exit -> invoke -> killing begin -> loopend/exit - * + *

* 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 diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopsData.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopsData.java index c262b4699a80..e70e8747ec3d 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/nodes/loop/LoopsData.java +++ b/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); } diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/ConditionalEliminationPhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/ConditionalEliminationPhase.java index d24992cdfda8..d888a1e323eb 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/common/ConditionalEliminationPhase.java +++ b/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); } diff --git a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/schedule/SchedulePhase.java b/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/schedule/SchedulePhase.java index 4a47fdd6f2a3..4c0d1c40db43 100644 --- a/compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/phases/schedule/SchedulePhase.java +++ b/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 currentNodeMap = graph.createNodeMap();