Skip to content

Commit 58a7f31

Browse files
msimacekmsimacek
committed
Address review feedback
1 parent 7465ad6 commit 58a7f31

File tree

6 files changed

+51
-53
lines changed

6 files changed

+51
-53
lines changed

‎graalpython/com.oracle.graal.python.pegparser/src/com/oracle/graal/python/pegparser/RuleResultCache.java

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -82,11 +82,7 @@ public T getResult(int pos, int ruleId) {
8282
}
8383

8484
public T putResult(int pos, int ruleId, T node) {
85-
HashMap<Integer, CachedItem<T>> posCache = mainCache.get(pos);
86-
if (posCache == null) {
87-
posCache = new HashMap<>();
88-
mainCache.put(pos, posCache);
89-
}
85+
HashMap<Integer, CachedItem<T>> posCache = mainCache.computeIfAbsent(pos, k -> new HashMap<>());
9086
posCache.put(ruleId, new CachedItem<>(node, parser.mark()));
9187
return node;
9288
}

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/builtins/objects/dict/DictNodes.java

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,6 @@ static void updateDict(PDict self, Object other,
103103
}
104104

105105
@Specialization(guards = {"!isDict(other)", "hasKeysAttr(frame, other, lookupKeys)"}, limit = "1")
106-
107106
static void updateMapping(VirtualFrame frame, PDict self, Object other,
108107
@SuppressWarnings("unused") @Shared("lookupKeys") @Cached PyObjectLookupAttr lookupKeys,
109108
@Shared("hlib") @CachedLibrary(limit = "3") HashingStorageLibrary lib,

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/PBytecodeRootNode.java

Lines changed: 38 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,11 @@ private void setVar(VirtualFrame virtualFrame, Object localsObject, PyObjectSetI
456456
}
457457

458458
private static FrameDescriptor makeFrameDescriptor(CodeUnit co) {
459-
FrameDescriptor.Builder newBuilder = FrameDescriptor.newBuilder(4);
459+
int capacity = co.varnames.length + co.cellvars.length + co.freevars.length + co.stacksize + 1;
460+
if (co.isGeneratorOrCoroutine()) {
461+
capacity += 2;
462+
}
463+
FrameDescriptor.Builder newBuilder = FrameDescriptor.newBuilder(capacity);
460464
newBuilder.info(new FrameInfo());
461465
// locals
462466
newBuilder.addSlots(co.varnames.length, FrameSlotKind.Illegal);
@@ -472,7 +476,7 @@ private static FrameDescriptor makeFrameDescriptor(CodeUnit co) {
472476
// stackTop saved when pausing a generator
473477
newBuilder.addSlot(FrameSlotKind.Int, null, null);
474478
// return value of a generator
475-
newBuilder.addSlot(FrameSlotKind.Int, null, null);
479+
newBuilder.addSlot(FrameSlotKind.Illegal, null, null);
476480
}
477481
return newBuilder.build();
478482
}
@@ -630,17 +634,17 @@ private <A, T extends Node> T doInsertChildNode(Node[] nodes, int nodeIndex, Nod
630634
}
631635

632636
@SuppressWarnings("unchecked")
633-
private <T extends Node> T insertChildNode(Node[] nodes, int nodeIndex, IntNodeFunction<T> nodeSupplier, int argument) {
637+
private <T extends Node> T insertChildNodeInt(Node[] nodes, int nodeIndex, IntNodeFunction<T> nodeSupplier, int argument) {
634638
Node node = nodes[nodeIndex];
635639
if (node != null) {
636640
return (T) node;
637641
}
638-
return doInsertChildNode(nodes, nodeIndex, nodeSupplier, argument);
642+
return doInsertChildNodeInt(nodes, nodeIndex, nodeSupplier, argument);
639643
}
640644

641645
@BytecodeInterpreterSwitchBoundary
642646
@SuppressWarnings("unchecked")
643-
private <T extends Node> T doInsertChildNode(Node[] nodes, int nodeIndex, IntNodeFunction<T> nodeSupplier, int argument) {
647+
private <T extends Node> T doInsertChildNodeInt(Node[] nodes, int nodeIndex, IntNodeFunction<T> nodeSupplier, int argument) {
644648
CompilerDirectives.transferToInterpreterAndInvalidate();
645649
T newNode = nodeSupplier.apply(argument);
646650
nodes[nodeIndex] = insert(newNode);
@@ -717,8 +721,8 @@ private static int encodeStackTop(int stackTop) {
717721

718722
@ExplodeLoop
719723
private void copyArgs(Object[] args, Frame localFrame) {
720-
for (int i = 0; i < PArguments.getUserArgumentLength(args); i++) {
721-
// we can set these as object, since they're already boxed
724+
int argCount = co.argCount + co.positionalOnlyArgCount + co.kwOnlyArgCount;
725+
for (int i = 0; i < argCount; i++) {
722726
localFrame.setObject(i, args[i + PArguments.USER_ARGUMENTS_OFFSET]);
723727
}
724728
}
@@ -774,8 +778,6 @@ public Object executeOSR(VirtualFrame osrFrame, int target, Object interpreterSt
774778
@ExplodeLoop(kind = ExplodeLoop.LoopExplosionKind.MERGE_EXPLODE)
775779
@SuppressWarnings("fallthrough")
776780
private Object executeInner(VirtualFrame virtualFrame, boolean resumingAfterOSR, int initialBci, int initialStackTop) {
777-
boolean inInterpreter = CompilerDirectives.inInterpreter();
778-
779781
Object globals = PArguments.getGlobals(virtualFrame);
780782
Object locals = PArguments.getSpecialArgument(virtualFrame);
781783

@@ -800,14 +802,18 @@ private Object executeInner(VirtualFrame virtualFrame, boolean resumingAfterOSR,
800802
String[] localNames = names;
801803
Node[] localNodes = adoptedNodes;
802804

803-
verifyBeforeLoop(stackTop, bci, localBC);
805+
CompilerAsserts.partialEvaluationConstant(localBC);
806+
CompilerAsserts.partialEvaluationConstant(bci);
807+
CompilerAsserts.partialEvaluationConstant(stackTop);
804808

805809
int oparg = 0;
806810
while (true) {
807811
final byte bc = localBC[bci];
808812
final int beginBci = bci;
809813

810-
verifyInLoop(stackTop, bci, bc);
814+
CompilerAsserts.partialEvaluationConstant(bc);
815+
CompilerAsserts.partialEvaluationConstant(bci);
816+
CompilerAsserts.partialEvaluationConstant(stackTop);
811817

812818
try {
813819
switch (bc) {
@@ -1000,15 +1006,15 @@ private Object executeInner(VirtualFrame virtualFrame, boolean resumingAfterOSR,
10001006
break;
10011007
case OpCodesConstants.UNARY_OP: {
10021008
int op = Byte.toUnsignedInt(localBC[++bci]);
1003-
UnaryOpNode opNode = insertChildNode(localNodes, bci, UNARY_OP_FACTORY, op);
1009+
UnaryOpNode opNode = insertChildNodeInt(localNodes, bci, UNARY_OP_FACTORY, op);
10041010
Object value = localFrame.getObject(stackTop);
10051011
Object result = opNode.execute(virtualFrame, value);
10061012
localFrame.setObject(stackTop, result);
10071013
break;
10081014
}
10091015
case OpCodesConstants.BINARY_OP: {
10101016
int op = Byte.toUnsignedInt(localBC[++bci]);
1011-
BinaryOp opNode = (BinaryOp) insertChildNode(localNodes, bci, BINARY_OP_FACTORY, op);
1017+
BinaryOp opNode = (BinaryOp) insertChildNodeInt(localNodes, bci, BINARY_OP_FACTORY, op);
10121018
Object right = localFrame.getObject(stackTop);
10131019
localFrame.setObject(stackTop--, null);
10141020
Object left = localFrame.getObject(stackTop);
@@ -1037,7 +1043,7 @@ private Object executeInner(VirtualFrame virtualFrame, boolean resumingAfterOSR,
10371043
break;
10381044
}
10391045
case OpCodesConstants.RETURN_VALUE: {
1040-
if (inInterpreter) {
1046+
if (CompilerDirectives.hasNextTier() && loopCount > 0) {
10411047
LoopNode.reportLoopCount(this, loopCount);
10421048
}
10431049
Object value = localFrame.getObject(stackTop);
@@ -1189,24 +1195,26 @@ private Object executeInner(VirtualFrame virtualFrame, boolean resumingAfterOSR,
11891195
case OpCodesConstants.JUMP_BACKWARD: {
11901196
oparg |= Byte.toUnsignedInt(localBC[bci + 1]);
11911197
bci -= oparg;
1192-
if (inInterpreter) {
1198+
if (CompilerDirectives.hasNextTier()) {
11931199
loopCount++;
1194-
if (BytecodeOSRNode.pollOSRBackEdge(this)) {
1195-
/*
1196-
* Beware of race conditions when adding more things to the
1197-
* interpreterState argument. It gets stored already at this point,
1198-
* but the compilation runs in parallel. The compiled code may get
1199-
* entered from a different invocation of this root, using the
1200-
* interpreterState that was saved here. Don't put any data specific
1201-
* to particular invocation in there (like python-level arguments or
1202-
* variables) or it will get mixed up. To retain such state, put it
1203-
* into the frame instead.
1204-
*/
1205-
Object osrResult = BytecodeOSRNode.tryOSR(this, bci, stackTop, null, virtualFrame);
1206-
if (osrResult != null) {
1200+
}
1201+
if (CompilerDirectives.inInterpreter() && BytecodeOSRNode.pollOSRBackEdge(this)) {
1202+
/*
1203+
* Beware of race conditions when adding more things to the
1204+
* interpreterState argument. It gets stored already at this point, but
1205+
* the compilation runs in parallel. The compiled code may get entered
1206+
* from a different invocation of this root, using the interpreterState
1207+
* that was saved here. Don't put any data specific to particular
1208+
* invocation in there (like python-level arguments or variables) or it
1209+
* will get mixed up. To retain such state, put it into the frame
1210+
* instead.
1211+
*/
1212+
Object osrResult = BytecodeOSRNode.tryOSR(this, bci, stackTop, null, virtualFrame);
1213+
if (osrResult != null) {
1214+
if (CompilerDirectives.hasNextTier() && loopCount > 0) {
12071215
LoopNode.reportLoopCount(this, loopCount);
1208-
return osrResult;
12091216
}
1217+
return osrResult;
12101218
}
12111219
}
12121220
oparg = 0;
@@ -1325,7 +1333,7 @@ private Object executeInner(VirtualFrame virtualFrame, boolean resumingAfterOSR,
13251333
throw bytecodeEndExcHandler(virtualFrame, localFrame, stackTop);
13261334
}
13271335
case OpCodesConstants.YIELD_VALUE: {
1328-
if (inInterpreter) {
1336+
if (CompilerDirectives.hasNextTier() && loopCount > 0) {
13291337
LoopNode.reportLoopCount(this, loopCount);
13301338
}
13311339
Object value = localFrame.getObject(stackTop);
@@ -1889,18 +1897,6 @@ private void initFreeVarsLoop(Frame localFrame, Object[] originalArgs) {
18891897
}
18901898
}
18911899

1892-
private void verifyInLoop(int stackTop, int bci, final byte bc) {
1893-
CompilerAsserts.partialEvaluationConstant(bc);
1894-
CompilerAsserts.partialEvaluationConstant(bci);
1895-
CompilerAsserts.partialEvaluationConstant(stackTop);
1896-
}
1897-
1898-
private void verifyBeforeLoop(int stackTop, int bci, byte[] localBC) {
1899-
CompilerAsserts.partialEvaluationConstant(localBC);
1900-
CompilerAsserts.partialEvaluationConstant(bci);
1901-
CompilerAsserts.partialEvaluationConstant(stackTop);
1902-
}
1903-
19041900
@ExplodeLoop
19051901
@SuppressWarnings("unchecked")
19061902
private static <T> void moveFromStack(Frame localFrame, int start, int stop, T[] target) {
@@ -2109,9 +2105,6 @@ private int bytecodeUnpackEx(VirtualFrame virtualFrame, Frame localFrame, int st
21092105
return unpackNode.execute(virtualFrame, stackTop - 1, localFrame, collection, countBefore, countAfter);
21102106
}
21112107

2112-
/**
2113-
* @see #saveExceptionBlockstack
2114-
*/
21152108
@ExplodeLoop
21162109
private long findHandler(int bci) {
21172110
CompilerAsserts.partialEvaluationConstant(bci);

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/UnpackExNode.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858
import com.oracle.graal.python.runtime.object.PythonObjectFactory;
5959
import com.oracle.graal.python.runtime.sequence.PSequence;
6060
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
61+
import com.oracle.truffle.api.CompilerAsserts;
6162
import com.oracle.truffle.api.dsl.Cached;
6263
import com.oracle.truffle.api.dsl.Cached.Shared;
6364
import com.oracle.truffle.api.dsl.Fallback;
@@ -82,6 +83,8 @@ static int doUnpackSequence(int initialStackTop, Frame localFrame, PSequence seq
8283
@Cached BranchProfile errorProfile,
8384
@Shared("factory") @Cached PythonObjectFactory factory,
8485
@Shared("raise") @Cached PRaiseNode raiseNode) {
86+
CompilerAsserts.partialEvaluationConstant(countBefore);
87+
CompilerAsserts.partialEvaluationConstant(countAfter);
8588
int resultStackTop = initialStackTop + countBefore + 1 + countAfter;
8689
int stackTop = resultStackTop;
8790
SequenceStorage storage = getSequenceStorageNode.execute(sequence);
@@ -110,6 +113,8 @@ static int doUnpackIterable(VirtualFrame virtualFrame, int initialStackTop, Fram
110113
@Cached SequenceStorageNodes.GetItemSliceNode getItemSliceNode,
111114
@Shared("factory") @Cached PythonObjectFactory factory,
112115
@Shared("raise") @Cached PRaiseNode raiseNode) {
116+
CompilerAsserts.partialEvaluationConstant(countBefore);
117+
CompilerAsserts.partialEvaluationConstant(countAfter);
113118
int resultStackTop = initialStackTop + countBefore + 1 + countAfter;
114119
int stackTop = resultStackTop;
115120
Object iterator;
@@ -140,6 +145,7 @@ static int doUnpackIterable(VirtualFrame virtualFrame, int initialStackTop, Fram
140145
@ExplodeLoop
141146
private static int moveItemsToStack(VirtualFrame virtualFrame, Object iterator, Frame localFrame, int initialStackTop, int offset, int length, int totalLength, GetNextNode getNextNode,
142147
IsBuiltinClassProfile stopIterationProfile, PRaiseNode raiseNode) {
148+
CompilerAsserts.partialEvaluationConstant(length);
143149
int stackTop = initialStackTop;
144150
for (int i = 0; i < length; i++) {
145151
try {
@@ -155,6 +161,7 @@ private static int moveItemsToStack(VirtualFrame virtualFrame, Object iterator,
155161

156162
@ExplodeLoop
157163
private static int moveItemsToStack(SequenceStorage storage, Frame localFrame, int initialStackTop, int offset, int length, SequenceStorageNodes.GetItemScalarNode getItemNode) {
164+
CompilerAsserts.partialEvaluationConstant(length);
158165
int stackTop = initialStackTop;
159166
for (int i = 0; i < length; i++) {
160167
localFrame.setObject(stackTop--, getItemNode.execute(storage, offset + i));

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode/UnpackSequenceNode.java

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@
5555
import com.oracle.graal.python.runtime.exception.PException;
5656
import com.oracle.graal.python.runtime.sequence.PSequence;
5757
import com.oracle.graal.python.runtime.sequence.storage.SequenceStorage;
58+
import com.oracle.truffle.api.CompilerAsserts;
5859
import com.oracle.truffle.api.dsl.Cached;
5960
import com.oracle.truffle.api.dsl.Cached.Shared;
6061
import com.oracle.truffle.api.dsl.Fallback;
@@ -77,6 +78,7 @@ static int doUnpackSequence(int initialStackTop, Frame localFrame, PSequence seq
7778
@Cached SequenceStorageNodes.GetItemScalarNode getItemNode,
7879
@Cached BranchProfile errorProfile,
7980
@Shared("raise") @Cached PRaiseNode raiseNode) {
81+
CompilerAsserts.partialEvaluationConstant(count);
8082
int resultStackTop = initialStackTop + count;
8183
int stackTop = resultStackTop;
8284
SequenceStorage storage = getSequenceStorageNode.execute(sequence);
@@ -105,6 +107,7 @@ static int doUnpackIterable(Frame virtualFrame, int initialStackTop, Frame local
105107
@Cached IsBuiltinClassProfile stopIterationProfile1,
106108
@Cached IsBuiltinClassProfile stopIterationProfile2,
107109
@Shared("raise") @Cached PRaiseNode raiseNode) {
110+
CompilerAsserts.partialEvaluationConstant(count);
108111
int resultStackTop = initialStackTop + count;
109112
int stackTop = resultStackTop;
110113
Object iterator;

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/frame/ReadGlobalOrBuiltinNode.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@
6767
public abstract class ReadGlobalOrBuiltinNode extends ExpressionNode implements ReadNode, GlobalNode {
6868
private static final ReadGlobalOrBuiltinNode UNCACHED = new ReadGlobalOrBuiltinNode(null) {
6969
@Override
70-
public Object executeWithGlobals(VirtualFrame frame, Object globals) {
70+
protected Object executeWithGlobals(VirtualFrame frame, Object globals) {
7171
throw CompilerDirectives.shouldNotReachHere("uncached ReadGlobalOrBuiltinNode must be used with #read");
7272
}
7373

@@ -141,7 +141,7 @@ public final boolean executeBoolean(VirtualFrame frame) throws UnexpectedResultE
141141
throw new UnexpectedResultException(o);
142142
}
143143

144-
public abstract Object executeWithGlobals(VirtualFrame frame, Object globals);
144+
protected abstract Object executeWithGlobals(VirtualFrame frame, Object globals);
145145

146146
public Object read(Frame frame, Object globals, String name) {
147147
assert name == attributeId : "cached ReadGlobalOrBuiltinNode can only be used with cached attributeId";

0 commit comments

Comments
 (0)