Skip to content

Commit e259190

Browse files
gilles-duboscqgilles-duboscq
committed
[GR-37100] Fix frame caching and frame transfer for BytecodeOSR
PullRequest: graal/11536
2 parents 04f97b4 + f36058b commit e259190

File tree

17 files changed

+812
-257
lines changed

17 files changed

+812
-257
lines changed

‎compiler/src/org.graalvm.compiler.truffle.runtime/src/org/graalvm/compiler/truffle/runtime/BytecodeOSRMetadata.java‎

Lines changed: 342 additions & 141 deletions
Large diffs are not rendered by default.

‎compiler/src/org.graalvm.compiler.truffle.runtime/src/org/graalvm/compiler/truffle/runtime/BytecodeOSRRootNode.java‎

Lines changed: 67 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,31 +24,47 @@
2424
*/
2525
package org.graalvm.compiler.truffle.runtime;
2626

27+
import java.lang.reflect.Method;
28+
import java.util.Map;
29+
import java.util.concurrent.ConcurrentHashMap;
30+
2731
import com.oracle.truffle.api.CompilerDirectives;
2832
import com.oracle.truffle.api.CompilerDirectives.CompilationFinal;
2933
import com.oracle.truffle.api.Truffle;
3034
import com.oracle.truffle.api.TruffleLanguage;
3135
import com.oracle.truffle.api.frame.FrameDescriptor;
3236
import com.oracle.truffle.api.frame.VirtualFrame;
3337
import com.oracle.truffle.api.nodes.BytecodeOSRNode;
38+
import org.graalvm.nativeimage.ImageInfo;
3439

3540
final class BytecodeOSRRootNode extends BaseOSRRootNode {
3641
private final int target;
3742
private final Object interpreterState;
3843
@CompilationFinal private boolean seenMaterializedFrame;
3944

40-
BytecodeOSRRootNode(TruffleLanguage<?> language, FrameDescriptor frameDescriptor, BytecodeOSRNode bytecodeOSRNode, int target, Object interpreterState) {
45+
private final Object entryTagsCache;
46+
47+
BytecodeOSRRootNode(TruffleLanguage<?> language, FrameDescriptor frameDescriptor, BytecodeOSRNode bytecodeOSRNode, int target, Object interpreterState, Object entryTagsCache) {
4148
super(language, frameDescriptor, bytecodeOSRNode);
4249
this.target = target;
4350
this.interpreterState = interpreterState;
4451
this.seenMaterializedFrame = materializeCalled(frameDescriptor);
52+
this.entryTagsCache = entryTagsCache;
53+
54+
// Support for deprecated frame transfer: GR-38296
55+
this.usesDeprecatedFrameTransfer = checkUsesDeprecatedFrameTransfer(bytecodeOSRNode.getClass());
4556
}
4657

4758
private static boolean materializeCalled(FrameDescriptor frameDescriptor) {
4859
return ((GraalTruffleRuntime) Truffle.getRuntime()).getFrameMaterializeCalled(frameDescriptor);
4960
}
5061

62+
Object getEntryTagsCache() {
63+
return entryTagsCache;
64+
}
65+
5166
@Override
67+
@SuppressWarnings("deprecation")
5268
public Object executeOSR(VirtualFrame frame) {
5369
VirtualFrame parentFrame = (VirtualFrame) frame.getArguments()[0];
5470

@@ -67,7 +83,11 @@ public Object executeOSR(VirtualFrame frame) {
6783
// required to prevent the materialized frame from getting out of sync during OSR.
6884
return osrNode.executeOSR(parentFrame, target, interpreterState);
6985
} else {
70-
osrNode.copyIntoOSRFrame(frame, parentFrame, target);
86+
if (usesDeprecatedFrameTransfer) { // Support for deprecated frame transfer: GR-38296
87+
osrNode.copyIntoOSRFrame(frame, parentFrame, target);
88+
} else {
89+
osrNode.copyIntoOSRFrame(frame, parentFrame, target, entryTagsCache);
90+
}
7191
try {
7292
return osrNode.executeOSR(frame, target, interpreterState);
7393
} finally {
@@ -85,4 +105,49 @@ public String getName() {
85105
public String toString() {
86106
return loopNode.toString() + "<OSR@" + target + ">";
87107
}
108+
109+
// GR-38296
110+
/* Deprecated frame transfer handling below */
111+
112+
private static final Map<Class<?>, Boolean> usesDeprecatedTransferClasses = new ConcurrentHashMap<>();
113+
114+
private final boolean usesDeprecatedFrameTransfer;
115+
116+
/**
117+
* Detects usage of deprecated frame transfer, and directs the frame transfer path accordingly
118+
* later. When removing the support for this deprecation, constructs used and paths related are
119+
* marked with the comment "Support for deprecated frame transfer" and a reference to GR-38296.
120+
*/
121+
private static boolean usesDeprecatedFrameTransfer(Class<?> osrNodeClass) {
122+
try {
123+
Method m = osrNodeClass.getMethod("copyIntoOSRFrame", VirtualFrame.class, VirtualFrame.class, int.class);
124+
return m.getDeclaringClass() != BytecodeOSRNode.class;
125+
} catch (NoSuchMethodException e) {
126+
return false;
127+
}
128+
}
129+
130+
private static boolean checkUsesDeprecatedFrameTransfer(Class<?> osrNodeClass) {
131+
if (ImageInfo.inImageRuntimeCode()) {
132+
// this must have been pre-computed
133+
return usesDeprecatedTransferClasses.get(osrNodeClass);
134+
} else {
135+
return usesDeprecatedTransferClasses.computeIfAbsent(osrNodeClass, BytecodeOSRRootNode::usesDeprecatedFrameTransfer);
136+
}
137+
}
138+
139+
// Called by truffle feature to initialize the map at build time.
140+
@SuppressWarnings("unused")
141+
private static boolean initializeClassUsingDeprecatedFrameTransfer(Class<?> subType) {
142+
if (subType.isInterface()) {
143+
return false;
144+
}
145+
if (usesDeprecatedTransferClasses.containsKey(subType)) {
146+
return false;
147+
}
148+
// Eagerly initialize result.
149+
usesDeprecatedTransferClasses.put(subType, usesDeprecatedFrameTransfer(subType));
150+
return true;
151+
}
152+
88153
}

‎compiler/src/org.graalvm.compiler.truffle.runtime/src/org/graalvm/compiler/truffle/runtime/GraalRuntimeSupport.java‎

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,8 +39,8 @@
3939
import com.oracle.truffle.api.frame.Frame;
4040
import com.oracle.truffle.api.frame.VirtualFrame;
4141
import com.oracle.truffle.api.impl.AbstractFastThreadLocal;
42-
import com.oracle.truffle.api.impl.FrameWithoutBoxing;
4342
import com.oracle.truffle.api.impl.Accessor.RuntimeSupport;
43+
import com.oracle.truffle.api.impl.FrameWithoutBoxing;
4444
import com.oracle.truffle.api.impl.ThreadLocalHandshake;
4545
import com.oracle.truffle.api.nodes.BlockNode;
4646
import com.oracle.truffle.api.nodes.BlockNode.ElementExecutor;
@@ -158,10 +158,24 @@ public void onOSRNodeReplaced(BytecodeOSRNode osrNode, Node oldNode, Node newNod
158158
}
159159
}
160160

161+
// Support for deprecated frame transfer: GR-38296
162+
@Override
163+
public void transferOSRFrame(BytecodeOSRNode osrNode, Frame source, Frame target, int bytecodeTarget) {
164+
BytecodeOSRMetadata osrMetadata = (BytecodeOSRMetadata) osrNode.getOSRMetadata();
165+
BytecodeOSRMetadata.OsrEntryDescription targetMetadata = osrMetadata.getLazyState().get(bytecodeTarget);
166+
osrMetadata.transferFrame((FrameWithoutBoxing) source, (FrameWithoutBoxing) target, bytecodeTarget, targetMetadata);
167+
}
168+
169+
@Override
170+
public void transferOSRFrame(BytecodeOSRNode osrNode, Frame source, Frame target, int bytecodeTarget, Object targetMetadata) {
171+
BytecodeOSRMetadata osrMetadata = (BytecodeOSRMetadata) osrNode.getOSRMetadata();
172+
osrMetadata.transferFrame((FrameWithoutBoxing) source, (FrameWithoutBoxing) target, bytecodeTarget, targetMetadata);
173+
}
174+
161175
@Override
162-
public void transferOSRFrame(BytecodeOSRNode osrNode, Frame source, Frame target) {
176+
public void restoreOSRFrame(BytecodeOSRNode osrNode, Frame source, Frame target) {
163177
BytecodeOSRMetadata osrMetadata = (BytecodeOSRMetadata) osrNode.getOSRMetadata();
164-
osrMetadata.transferFrame((FrameWithoutBoxing) source, (FrameWithoutBoxing) target);
178+
osrMetadata.restoreFrame((FrameWithoutBoxing) source, (FrameWithoutBoxing) target);
165179
}
166180

167181
@Override

‎compiler/src/org.graalvm.compiler.truffle.test/src/org/graalvm/compiler/truffle/test/BytecodeOSRNodeTest.java‎

Lines changed: 108 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
import com.oracle.truffle.api.frame.FrameSlotKind;
5353
import com.oracle.truffle.api.frame.FrameSlotTypeException;
5454
import com.oracle.truffle.api.frame.VirtualFrame;
55-
import com.oracle.truffle.api.impl.FrameWithoutBoxing;
5655
import com.oracle.truffle.api.nodes.BytecodeOSRNode;
5756
import com.oracle.truffle.api.nodes.ExplodeLoop;
5857
import com.oracle.truffle.api.nodes.Node;
@@ -138,6 +137,21 @@ public void testMultipleLoops() {
138137
Assert.assertEquals(TwoFixedIterationLoops.NO_OSR, target.call(osrThreshold / 2));
139138
}
140139

140+
/*
141+
* Test that OSR is triggered in the expected location when multiple loops are involved, and
142+
* makes sure that their cached frame states for transfers are independent.
143+
*/
144+
@Test
145+
public void testMultipleLoopsIncompatibleState() {
146+
// Each loop runs for osrThreshold + 1 iterations, so the first loop should trigger OSR.
147+
FrameDescriptor.Builder builder = FrameDescriptor.newBuilder();
148+
TwoLoopsIncompatibleFrames osrNode = new TwoLoopsIncompatibleFrames(builder);
149+
RootNode rootNode = new Program(osrNode, builder.build());
150+
OptimizedCallTarget target = (OptimizedCallTarget) rootNode.getCallTarget();
151+
Assert.assertEquals(TwoLoopsIncompatibleFrames.OSR_IN_FIRST_LOOP, target.call(osrThreshold + 1, true));
152+
Assert.assertEquals(TwoLoopsIncompatibleFrames.OSR_IN_SECOND_LOOP, target.call(osrThreshold + 1, false));
153+
}
154+
141155
/*
142156
* Test that OSR fails if the code cannot be compiled.
143157
*/
@@ -576,7 +590,7 @@ public FixedIterationLoop(FrameDescriptor frameDescriptor) {
576590
}
577591

578592
@Override
579-
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target) {
593+
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target, Object targetMetadata) {
580594
setInt(osrFrame, indexSlot, getInt(parentFrame, indexSlot));
581595
setInt(osrFrame, numIterationsSlot, getInt(parentFrame, numIterationsSlot));
582596
}
@@ -657,6 +671,90 @@ protected Object executeLoop(VirtualFrame frame, int numIterations) {
657671
}
658672
}
659673

674+
/**
675+
* Contains two loops whose entry's frame states differs. Makes sure that one's entry frame
676+
* state does not spill to the other
677+
*/
678+
public static class TwoLoopsIncompatibleFrames extends BytecodeOSRTestNode {
679+
static final String NO_OSR = "no osr";
680+
static final String OSR_IN_FIRST_LOOP = "osr in first loop";
681+
static final String OSR_IN_SECOND_LOOP = "osr in second loop";
682+
683+
static final int FIRST_LOOP_TARGET = 0;
684+
static final int SECOND_LOOP_TARGET = 1;
685+
686+
@CompilationFinal int iterationsSlot;
687+
@CompilationFinal int indexSlot;
688+
@CompilationFinal int selectSlot;
689+
@CompilationFinal int localSlot;
690+
691+
public TwoLoopsIncompatibleFrames(FrameDescriptor.Builder builder) {
692+
iterationsSlot = builder.addSlot(FrameSlotKind.Int, "iterations", null);
693+
indexSlot = builder.addSlot(FrameSlotKind.Int, "index", null);
694+
selectSlot = builder.addSlot(FrameSlotKind.Boolean, "select", null);
695+
localSlot = builder.addSlot(FrameSlotKind.Illegal, "local", null);
696+
}
697+
698+
@Override
699+
Object execute(VirtualFrame frame) {
700+
int numIter = (int) frame.getArguments()[0];
701+
boolean select = (boolean) frame.getArguments()[1];
702+
frame.setInt(iterationsSlot, numIter);
703+
frame.setInt(indexSlot, 0);
704+
frame.setBoolean(selectSlot, select);
705+
if (select) {
706+
frame.setInt(localSlot, 0);
707+
return executeLoop(frame, numIter, select);
708+
} else {
709+
frame.setDouble(localSlot, 0d);
710+
return executeLoop(frame, numIter, select);
711+
}
712+
}
713+
714+
@Override
715+
public Object executeOSR(VirtualFrame osrFrame, int target, Object interpreterState) {
716+
return executeLoop(osrFrame, osrFrame.getInt(iterationsSlot), target == FIRST_LOOP_TARGET);
717+
}
718+
719+
protected Object executeLoop(VirtualFrame frame, int numIterations, boolean select) {
720+
try {
721+
if (select) {
722+
for (int i = frame.getInt(indexSlot); i < numIterations; i++) {
723+
frame.setInt(indexSlot, i);
724+
int partial = frame.getInt(localSlot);
725+
frame.setInt(localSlot, i + partial);
726+
if (i + 1 < numIterations) { // back-edge will be taken
727+
if (BytecodeOSRNode.pollOSRBackEdge(this)) {
728+
Object result = BytecodeOSRNode.tryOSR(this, FIRST_LOOP_TARGET, null, null, frame);
729+
if (result != null) {
730+
return OSR_IN_FIRST_LOOP;
731+
}
732+
}
733+
}
734+
}
735+
} else {
736+
for (int i = frame.getInt(indexSlot); i < numIterations; i++) {
737+
frame.setInt(indexSlot, i);
738+
double partial = frame.getDouble(localSlot);
739+
frame.setDouble(localSlot, i + partial);
740+
if (i + 1 < numIterations) { // back-edge will be taken
741+
if (BytecodeOSRNode.pollOSRBackEdge(this)) {
742+
Object result = BytecodeOSRNode.tryOSR(this, SECOND_LOOP_TARGET, null, null, frame);
743+
if (result != null) {
744+
return OSR_IN_SECOND_LOOP;
745+
}
746+
}
747+
}
748+
}
749+
}
750+
return NO_OSR;
751+
} catch (FrameSlotTypeException e) {
752+
CompilerDirectives.transferToInterpreter();
753+
throw new IllegalStateException("Error accessing index slot");
754+
}
755+
}
756+
}
757+
660758
public static class UncompilableFixedIterationLoop extends FixedIterationLoop {
661759
public UncompilableFixedIterationLoop(FrameDescriptor frameDescriptor) {
662760
super(frameDescriptor);
@@ -1013,8 +1111,8 @@ public Object executeOSR(VirtualFrame osrFrame, int target, Object interpreterSt
10131111
}
10141112

10151113
@Override
1016-
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target) {
1017-
super.copyIntoOSRFrame(osrFrame, parentFrame, target);
1114+
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target, Object targetMetadata) {
1115+
super.copyIntoOSRFrame(osrFrame, parentFrame, target, targetMetadata);
10181116
// Copying should not trigger a deopt.
10191117
Assert.assertTrue(CompilerDirectives.inCompiledCode());
10201118
}
@@ -1128,10 +1226,9 @@ public void checkOSRState(VirtualFrame frame) {
11281226
public void restoreParentFrame(VirtualFrame osrFrame, VirtualFrame parentFrame) {
11291227
// The parent implementation asserts we are in compiled code, so we instead explicitly
11301228
// do the transfer here.
1131-
((BytecodeOSRMetadata) osrMetadata).transferFrame((FrameWithoutBoxing) osrFrame, (FrameWithoutBoxing) parentFrame);
1132-
// Since the intSlot tag changed inside the compiled code, the tag speculation should
1133-
// fail and cause a deopt.
1134-
Assert.assertFalse(CompilerDirectives.inCompiledCode());
1229+
super.restoreParentFrame(osrFrame, parentFrame);
1230+
// Parent frame restoration does not speculate on the state of the frame on return.
1231+
Assert.assertTrue(CompilerDirectives.inCompiledCode());
11351232
}
11361233
}
11371234

@@ -1204,9 +1301,9 @@ public FrameChangingNode(FrameDescriptor frameDescriptor) {
12041301
}
12051302

12061303
@Override
1207-
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target) {
1304+
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target, Object targetMetadata) {
12081305
changeFrame(osrFrame.getFrameDescriptor()); // only changes the first time
1209-
super.copyIntoOSRFrame(osrFrame, parentFrame, target);
1306+
super.copyIntoOSRFrame(osrFrame, parentFrame, target, targetMetadata);
12101307
}
12111308

12121309
@Override
@@ -1335,7 +1432,7 @@ public Object execute(VirtualFrame frame) {
13351432

13361433
@Override
13371434
@ExplodeLoop
1338-
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target) {
1435+
public void copyIntoOSRFrame(VirtualFrame osrFrame, VirtualFrame parentFrame, int target, Object targetMetadata) {
13391436
for (int i = 0; i < regs.length; i++) {
13401437
setInt(osrFrame, i, getInt(parentFrame, i));
13411438
}

0 commit comments

Comments
 (0)