Skip to content

Commit 4b8f700

Browse files
medoussbougmedoussboug
committed
[GR-48971] Backport to 23.1: Ensure Guards Anchor to CaptureStateBegin
PullRequest: graal/15866
2 parents 6ab4a86 + 4669bcc commit 4b8f700

File tree

3 files changed

+90
-38
lines changed

3 files changed

+90
-38
lines changed

‎compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/nodes/extended/CaptureStateBeginNode.java

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,14 @@
3030

3131
import org.graalvm.compiler.graph.Node;
3232
import org.graalvm.compiler.graph.NodeClass;
33+
import org.graalvm.compiler.graph.iterators.NodePredicates;
3334
import org.graalvm.compiler.nodeinfo.NodeInfo;
3435
import org.graalvm.compiler.nodes.AbstractBeginNode;
3536
import org.graalvm.compiler.nodes.BeginNode;
3637
import org.graalvm.compiler.nodes.BeginStateSplitNode;
38+
import org.graalvm.compiler.nodes.LoopExitNode;
39+
import org.graalvm.compiler.nodes.MemoryProxyNode;
40+
import org.graalvm.compiler.nodes.ValueProxyNode;
3741
import org.graalvm.compiler.nodes.spi.Canonicalizable;
3842
import org.graalvm.compiler.nodes.spi.CanonicalizerTool;
3943

@@ -61,4 +65,17 @@ public Node canonical(CanonicalizerTool tool) {
6165
}
6266
return this;
6367
}
68+
69+
@Override
70+
public boolean verify() {
71+
if (predecessor() instanceof LoopExitNode loopExit) {
72+
/*
73+
* Must guarantee only value and memory proxies are attached to the loop exit. Anything
74+
* else should be attached to this node
75+
*/
76+
assert loopExit.usages().stream().allMatch(NodePredicates.isA(ValueProxyNode.class).or(MemoryProxyNode.class)) : String.format("LoopExit has disallowed usages %s", loopExit);
77+
}
78+
79+
return super.verify();
80+
}
6481
}

‎compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/ConditionalEliminationPhase.java

Lines changed: 60 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,7 @@
8585
import org.graalvm.compiler.nodes.calc.IntegerEqualsNode;
8686
import org.graalvm.compiler.nodes.cfg.ControlFlowGraph;
8787
import org.graalvm.compiler.nodes.cfg.HIRBlock;
88+
import org.graalvm.compiler.nodes.extended.CaptureStateBeginNode;
8889
import org.graalvm.compiler.nodes.extended.GuardingNode;
8990
import org.graalvm.compiler.nodes.extended.IntegerSwitchNode;
9091
import org.graalvm.compiler.nodes.extended.LoadHubNode;
@@ -226,11 +227,46 @@ public static class MoveGuardsUpwards implements ControlFlowGraph.RecursiveVisit
226227

227228
HIRBlock anchorBlock;
228229

230+
/**
231+
* Guards cannot be moved above CaptureStateBeginNodes in order to ensure deoptimizations
232+
* are always attached to valid FrameStates.
233+
*/
234+
private static boolean disallowUpwardGuardMovement(HIRBlock b) {
235+
return b.getBeginNode() instanceof CaptureStateBeginNode;
236+
}
237+
229238
@Override
230239
@SuppressWarnings("try")
231240
public HIRBlock enter(HIRBlock b) {
232241
HIRBlock oldAnchorBlock = anchorBlock;
233-
if (b.getDominator() == null || b.getDominator().getPostdominator() != b) {
242+
/*
243+
* The goal of this pass is to move guards upward while not introducing the guards on
244+
* new paths. At all points the anchorBlock must set so the following two invariants
245+
* hold:
246+
*
247+
* (1) The anchorBlock dominates the current block.
248+
*
249+
* (2) The current block post-dominates the anchorBlock.
250+
*
251+
* Note blocks are traversed in dominator tree order.
252+
*
253+
* anchorBlock must be set to the current block if:
254+
*
255+
* (1) The current block does not have a dominator (i.e., this is the start of a new
256+
* dominator tree walk).
257+
*
258+
* (2) The immediate dominator of current block is not post-dominated by this block. Due
259+
* to using a dominator tree traversal, this is equivalent to ensuring the current block
260+
* post-dominates the anchorBlock.
261+
*
262+
* (3) Guards are not allowed to move above this block. The can happen when dominator
263+
* blocks can have invalid FrameStates, such as when the block start is a
264+
* CaptureStateBeginNode.
265+
*/
266+
boolean updateAnchorBlock = b.getDominator() == null ||
267+
b.getDominator().getPostdominator() != b ||
268+
disallowUpwardGuardMovement(b);
269+
if (updateAnchorBlock) {
234270
// New anchor.
235271
anchorBlock = b;
236272
}
@@ -271,52 +307,51 @@ public HIRBlock enter(HIRBlock b) {
271307
* successors are loop exits, even of potentially different loops. Thus, we need
272308
* to ensure we see all possible loop exits involved for all loops.
273309
*/
274-
LoopExitNode trueSuccLex = trueSuccessor instanceof LoopExitNode ? (LoopExitNode) trueSuccessor : null;
275-
LoopExitNode falseSuccLex = falseSuccessor instanceof LoopExitNode ? (LoopExitNode) falseSuccessor : null;
276310
EconomicSet<LoopExitNode> allLoopsAllExits = null;
277-
if (trueSuccLex != null) {
311+
if (trueSuccessor instanceof LoopExitNode successor) {
278312
if (allLoopsAllExits == null) {
279313
allLoopsAllExits = EconomicSet.create();
280314
}
281-
allLoopsAllExits.addAll(trueSuccLex.loopBegin().loopExits());
282-
allLoopsAllExits.remove(trueSuccLex);
315+
allLoopsAllExits.addAll(successor.loopBegin().loopExits());
316+
allLoopsAllExits.remove(successor);
283317
}
284-
if (falseSuccLex != null) {
318+
if (falseSuccessor instanceof LoopExitNode successor) {
285319
if (allLoopsAllExits == null) {
286320
allLoopsAllExits = EconomicSet.create();
287321
}
288-
allLoopsAllExits.addAll(falseSuccLex.loopBegin().loopExits());
289-
allLoopsAllExits.remove(falseSuccLex);
322+
allLoopsAllExits.addAll(successor.loopBegin().loopExits());
323+
allLoopsAllExits.remove(successor);
290324
}
291325
if (allLoopsAllExits == null || allLoopsAllExits.isEmpty()) {
292-
for (GuardNode guard : falseSuccessor.guards().snapshot()) {
293-
GuardNode otherGuard = trueGuards.get(guard.getCondition());
294-
if (otherGuard != null && guard.isNegated() == otherGuard.isNegated()) {
295-
Speculation speculation = otherGuard.getSpeculation();
326+
for (GuardNode falseGuard : falseSuccessor.guards().snapshot()) {
327+
GuardNode trueGuard = trueGuards.get(falseGuard.getCondition());
328+
if (trueGuard != null && falseGuard.isNegated() == trueGuard.isNegated()) {
329+
Speculation speculation = trueGuard.getSpeculation();
296330
if (speculation == null) {
297-
speculation = guard.getSpeculation();
298-
} else if (guard.getSpeculation() != null && guard.getSpeculation() != speculation) {
331+
speculation = falseGuard.getSpeculation();
332+
} else if (falseGuard.getSpeculation() != null && falseGuard.getSpeculation() != speculation) {
299333
// Cannot optimize due to different speculations.
300334
continue;
301335
}
302-
try (DebugCloseable closeable = guard.withNodeSourcePosition()) {
303-
StructuredGraph graph = guard.graph();
304-
GuardNode newlyCreatedGuard = new GuardNode(guard.getCondition(), anchorBlock.getBeginNode(), guard.getReason(), guard.getAction(), guard.isNegated(), speculation,
305-
guard.getNoDeoptSuccessorPosition());
336+
try (DebugCloseable closeable = falseGuard.withNodeSourcePosition()) {
337+
StructuredGraph graph = falseGuard.graph();
338+
GuardNode newlyCreatedGuard = new GuardNode(falseGuard.getCondition(), anchorBlock.getBeginNode(), falseGuard.getReason(), falseGuard.getAction(),
339+
falseGuard.isNegated(), speculation,
340+
falseGuard.getNoDeoptSuccessorPosition());
306341
GuardNode newGuard = node.graph().unique(newlyCreatedGuard);
307-
if (otherGuard.isAlive()) {
342+
if (trueGuard.isAlive()) {
308343
if (trueSuccessor instanceof LoopExitNode && beginNode.graph().isBeforeStage(StageFlag.VALUE_PROXY_REMOVAL)) {
309-
otherGuard.replaceAndDelete(ProxyNode.forGuard(newGuard, (LoopExitNode) trueSuccessor));
344+
trueGuard.replaceAndDelete(ProxyNode.forGuard(newGuard, (LoopExitNode) trueSuccessor));
310345
} else {
311-
otherGuard.replaceAndDelete(newGuard);
346+
trueGuard.replaceAndDelete(newGuard);
312347
}
313348
}
314349
if (falseSuccessor instanceof LoopExitNode && beginNode.graph().isBeforeStage(StageFlag.VALUE_PROXY_REMOVAL)) {
315-
guard.replaceAndDelete(ProxyNode.forGuard(newGuard, (LoopExitNode) falseSuccessor));
350+
falseGuard.replaceAndDelete(ProxyNode.forGuard(newGuard, (LoopExitNode) falseSuccessor));
316351
} else {
317-
guard.replaceAndDelete(newGuard);
352+
falseGuard.replaceAndDelete(newGuard);
318353
}
319-
graph.getOptimizationLog().report(ConditionalEliminationPhase.class, "GuardCombination", guard);
354+
graph.getOptimizationLog().report(ConditionalEliminationPhase.class, "GuardCombination", falseGuard);
320355
}
321356
}
322357
}

‎compiler/src/jdk.internal.vm.compiler/src/org/graalvm/compiler/phases/common/SnippetFrameStateAssignment.java

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -184,43 +184,43 @@ protected NodeStateAssignment merge(AbstractMergeNode merge, List<NodeStateAssig
184184
* in its return values. If such a snippet is encountered the subsequent logic will
185185
* assign an invalid state to the merge.
186186
*/
187-
NodeStateAssignment bci = null;
187+
NodeStateAssignment mergeAssignment = null;
188188
forAllStates: for (int i = 0; i < states.size(); i++) {
189189
NodeStateAssignment assignment = states.get(i);
190190
switch (assignment) {
191191
case BEFORE_BCI:
192192
/* If we only see BEFORE_BCI, the result will be BEFORE_BCI. */
193-
if (bci == null) {
194-
bci = NodeStateAssignment.BEFORE_BCI;
193+
if (mergeAssignment == null) {
194+
mergeAssignment = NodeStateAssignment.BEFORE_BCI;
195195
}
196196
break;
197197
case AFTER_BCI:
198198
case AFTER_BCI_INVALID_FOR_DEOPTIMIZATION:
199199
/* If we see at least one kind of AFTER_BCI, the result will be the same. */
200-
if (bci == NodeStateAssignment.AFTER_BCI || bci == NodeStateAssignment.AFTER_BCI_INVALID_FOR_DEOPTIMIZATION) {
201-
GraalError.guarantee(bci == assignment, "Cannot mix valid and invalid AFTER_BCI versions");
200+
if (mergeAssignment == NodeStateAssignment.AFTER_BCI || mergeAssignment == NodeStateAssignment.AFTER_BCI_INVALID_FOR_DEOPTIMIZATION) {
201+
GraalError.guarantee(mergeAssignment == assignment, "Cannot mix valid and invalid AFTER_BCI versions");
202202
}
203-
bci = assignment;
203+
mergeAssignment = assignment;
204204
break;
205205
case AFTER_EXCEPTION_BCI:
206206
/* AFTER_EXCEPTION_BCI can only be merged with itself. */
207-
if (bci == null || bci == NodeStateAssignment.AFTER_EXCEPTION_BCI) {
208-
bci = NodeStateAssignment.AFTER_EXCEPTION_BCI;
207+
if (mergeAssignment == null || mergeAssignment == NodeStateAssignment.AFTER_EXCEPTION_BCI) {
208+
mergeAssignment = NodeStateAssignment.AFTER_EXCEPTION_BCI;
209209
break;
210210
}
211211
/* Cannot merge AFTER_EXCEPTION_BCI with anything else. */
212-
bci = NodeStateAssignment.INVALID;
212+
mergeAssignment = NodeStateAssignment.INVALID;
213213
break forAllStates;
214214
case INVALID:
215-
bci = NodeStateAssignment.INVALID;
215+
mergeAssignment = NodeStateAssignment.INVALID;
216216
break forAllStates;
217217
default:
218218
throw GraalError.shouldNotReachHere("Unhandled node state assignment: " + assignment + " at merge " + merge); // ExcludeFromJacocoGeneratedReport
219219
}
220220
}
221-
assert bci != null;
222-
stateMapping.put(merge, bci);
223-
return bci;
221+
assert mergeAssignment != null;
222+
stateMapping.put(merge, mergeAssignment);
223+
return mergeAssignment;
224224
}
225225

226226
@Override

0 commit comments

Comments
 (0)