Skip to content

Commit 992e005

Browse files
steve-ssteve-s
committed
[GR-79186] Avoid temporary locals if loop or comprehension target is fast local.
PullRequest: graalpython/4821
2 parents ae6bcf1 + 40df476 commit 992e005

3 files changed

Lines changed: 81 additions & 60 deletions

File tree

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/SSTUtils.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
2+
* Copyright (c) 2026, Oracle and/or its affiliates. All rights reserved.
33
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
44
*
55
* The Universal Permissive License (UPL), Version 1.0
@@ -97,6 +97,10 @@ public static void checkForbiddenName(ParserCallbacks parserCallbacks, SourceRan
9797
}
9898
}
9999

100+
public static boolean mayBeForbiddenName(String id) {
101+
return id.equals("__debug__");
102+
}
103+
100104
public static void checkSubscripter(ParserCallbacks parserCallbacks, ExprTy e) {
101105
if (e instanceof ExprTy.Constant) {
102106
switch (((ExprTy.Constant) e).value.kind) {

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/bytecode_dsl/RootNodeCompiler.java‎

Lines changed: 76 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import static com.oracle.graal.python.compiler.SSTUtils.checkForbiddenArgs;
4949
import static com.oracle.graal.python.compiler.SSTUtils.checkIndex;
5050
import static com.oracle.graal.python.compiler.SSTUtils.checkSubscripter;
51+
import static com.oracle.graal.python.compiler.SSTUtils.mayBeForbiddenName;
5152
import static com.oracle.graal.python.compiler.bytecode_dsl.BytecodeDSLCompilerUtils.COMPREHENSION_ARGS;
5253
import static com.oracle.graal.python.compiler.bytecode_dsl.BytecodeDSLCompilerUtils.NO_ARGS;
5354
import static com.oracle.graal.python.compiler.bytecode_dsl.BytecodeDSLCompilerUtils.TYPE_PARAMS_DEFAULTS;
@@ -762,8 +763,9 @@ public void reset() {
762763
prevSaveExceptionLocal = null;
763764
lastTracedLine = -1;
764765
this.inExceptStar = false;
766+
temporaryLocals.clear();
765767
if (ASSERTIONS_ENABLED) {
766-
temporaryLocals.clear();
768+
temporaryLocalsTraces.clear();
767769
}
768770
}
769771

@@ -795,14 +797,14 @@ void endRootNode(Builder b) {
795797
b.endRoot();
796798
endRootSourceSection(b);
797799
b.endSource();
798-
if (ASSERTIONS_ENABLED && !temporaryLocals.isEmpty()) {
800+
if (ASSERTIONS_ENABLED && !temporaryLocalsTraces.isEmpty()) {
799801
throw new AssertionError(this.qualName + "\n\n" + formatTempLocalsStackTraces());
800802
}
801803
}
802804

803805
private String formatTempLocalsStackTraces() {
804806
StringBuilder sb = new StringBuilder();
805-
for (Object v : temporaryLocals.values()) {
807+
for (Object v : temporaryLocalsTraces.values()) {
806808
if (v instanceof RuntimeException re) {
807809
sb.append("\n==================\n");
808810
StringWriter sw = new StringWriter();
@@ -825,44 +827,59 @@ private static boolean assertionsEnabled() {
825827
return enabled;
826828
}
827829

828-
private HashMap<BytecodeLocal, Object> temporaryLocals = ASSERTIONS_ENABLED ? new HashMap<>() : null;
830+
private HashMap<BytecodeLocal, Object> temporaryLocalsTraces = ASSERTIONS_ENABLED ? new HashMap<>() : null;
831+
private HashSet<BytecodeLocal> temporaryLocals = new HashSet<>();
829832

830833
private BytecodeLocal beginTemporaryLocal(Builder b) {
831834
BytecodeLocal local = b.createLocal();
835+
temporaryLocals.add(local);
836+
assert isTemporaryLocal(local);
832837
if (ASSERTIONS_ENABLED) {
833-
Object previous = temporaryLocals.put(local, TRACK_TEMP_LOCALS ? new RuntimeException() : local);
838+
Object previous = temporaryLocalsTraces.put(local, TRACK_TEMP_LOCALS ? new RuntimeException() : local);
834839
if (previous != null) {
835840
throw new AssertionError();
836841
}
837842
}
838843
return local;
839844
}
840845

846+
public BytecodeLocal beginTemporaryLocalOrGetLocal(ExprTy target, Builder b) {
847+
if (target instanceof ExprTy.Name nameExpr) {
848+
BytecodeLocal l = getFastLocal(nameExpr.id);
849+
if (l != null) {
850+
return l;
851+
}
852+
}
853+
return beginTemporaryLocal(b);
854+
}
855+
856+
public boolean isTemporaryLocal(BytecodeLocal local) {
857+
return temporaryLocals.contains(local);
858+
}
859+
841860
private void endTemporaryLocal(BytecodeLocal local, Builder b) {
842-
markTemporaryLocalCleared(local);
843-
b.emitClearLocal(local);
861+
if (isTemporaryLocal(local)) {
862+
markTemporaryLocalCleared(local);
863+
b.emitClearLocal(local);
864+
}
844865
}
845866

846867
private void loadAndEndTemporaryLocal(BytecodeLocal local, Builder b) {
847-
markTemporaryLocalCleared(local);
848-
b.emitLoadAndClearTempLocal(local);
868+
if (isTemporaryLocal(local)) {
869+
markTemporaryLocalCleared(local);
870+
b.emitLoadAndClearTempLocal(local);
871+
}
849872
}
850873

851874
private void markTemporaryLocalCleared(BytecodeLocal local) {
852-
if (ASSERTIONS_ENABLED) {
853-
if (temporaryLocals.remove(local) == null) {
854-
throw new AssertionError();
855-
}
875+
if (!temporaryLocals.remove(local)) {
876+
throw new AssertionError();
856877
}
857-
}
858-
859-
private BytecodeLocal checkTemporaryLocal(BytecodeLocal local) {
860878
if (ASSERTIONS_ENABLED) {
861-
if (local != null && !temporaryLocals.containsKey(local)) {
862-
throw new AssertionError("Temporary local was already cleared");
879+
if (temporaryLocalsTraces.remove(local) == null) {
880+
throw new AssertionError();
863881
}
864882
}
865-
return local;
866883
}
867884

868885
void emitTraceLineChecked(SSTNode node, Builder b) {
@@ -1382,7 +1399,7 @@ private void emitComprehension(ComprehensionTy[] generators, int index, Builder
13821399
statementCompiler.emitAsyncFor(iter, comp.target, null, true, index,
13831400
(stmtComp, idx) -> emitComprehensionBody(generators, idx, type, collection, accumulateProducer, stmtComp));
13841401
} else {
1385-
BytecodeLocal localValue = beginTemporaryLocal(b);
1402+
BytecodeLocal localValue = beginTemporaryLocalOrGetLocal(comp.target, b);
13861403

13871404
b.beginBlock();
13881405
b.beginBindStackValue();
@@ -1411,7 +1428,7 @@ private void emitComprehension(ComprehensionTy[] generators, int index, Builder
14111428

14121429
b.beginBlock();
14131430

1414-
comp.target.accept(statementCompiler.new StoreVisitor(() -> b.emitLoadLocal(localValue)));
1431+
statementCompiler.storeTemporaryLocalToTarget(localValue, comp.target, b);
14151432
emitComprehensionBody(generators, index, type, collection, accumulateProducer, statementCompiler);
14161433

14171434
b.endBlock();
@@ -1851,6 +1868,25 @@ private void emitNameOperation(String name, NameOperation op, Builder b) {
18511868
emitNameSlowOperation(mangled, op, b);
18521869
}
18531870

1871+
private BytecodeLocal getFastLocal(String name) {
1872+
if (mayBeForbiddenName(name)) {
1873+
return null;
1874+
}
1875+
String mangled = maybeMangle(name);
1876+
EnumSet<DefUse> uses = scope.getUseOfName(mangled);
1877+
if (uses != null) {
1878+
if (uses.contains(DefUse.Free) || uses.contains(DefUse.Cell)) {
1879+
return null;
1880+
} else if (uses.contains(DefUse.Local)) {
1881+
if (scope.isFunction()) {
1882+
assert varnames.containsKey(mangled) : String.format("scope analysis did not mark %s as a regular variable", mangled);
1883+
return locals.get(mangled);
1884+
}
1885+
}
1886+
}
1887+
return null;
1888+
}
1889+
18541890
private void emitReadLocal(String name, Builder b) {
18551891
emitNameOperation(name, NameOperation.Read, b);
18561892
}
@@ -2016,6 +2052,23 @@ private BytecodeLocal beginTemporaryLocal() {
20162052
return RootNodeCompiler.this.beginTemporaryLocal(b);
20172053
}
20182054

2055+
/**
2056+
* If the target expression is simple local variable, we can often just directly store into it.
2057+
* Otherwise, we create temporary local. {@link BytecodeLocal} instances returned by this should
2058+
* be passed to {@link #storeTemporaryLocalToTarget(BytecodeLocal, ExprTy, Builder)}.
2059+
*/
2060+
public BytecodeLocal beginTemporaryLocalOrGetLocal(ExprTy target, Builder b) {
2061+
return RootNodeCompiler.this.beginTemporaryLocalOrGetLocal(target, b);
2062+
}
2063+
2064+
private void storeTemporaryLocalToTarget(BytecodeLocal temporaryLocal, ExprTy target, Builder b) {
2065+
if (RootNodeCompiler.this.isTemporaryLocal(temporaryLocal)) {
2066+
target.accept(new StoreVisitor(() -> {
2067+
b.emitLoadLocal(temporaryLocal);
2068+
}));
2069+
}
2070+
}
2071+
20192072
private void endTemporaryLocal(BytecodeLocal local) {
20202073
RootNodeCompiler.this.endTemporaryLocal(local, b);
20212074
}
@@ -4328,7 +4381,7 @@ public Void visit(StmtTy.For node) {
43284381
inExceptStar = false;
43294382
b.beginBlock();
43304383

4331-
BytecodeLocal value = beginTemporaryLocal();
4384+
BytecodeLocal value = beginTemporaryLocalOrGetLocal(node.target, b);
43324385

43334386
b.beginBindStackValue();
43344387
b.beginGetIter();
@@ -4355,9 +4408,7 @@ public Void visit(StmtTy.For node) {
43554408
// body
43564409
b.beginBlock();
43574410
continueLabel = b.createLabel();
4358-
node.target.accept(new StoreVisitor(() -> {
4359-
b.emitLoadLocal(value);
4360-
}));
4411+
storeTemporaryLocalToTarget(value, node.target, b);
43614412

43624413
visitSequence(node.body);
43634414
b.emitLabel(continueLabel);

‎graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode_dsl/PBytecodeDSLRootNode.java‎

Lines changed: 0 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1874,7 +1874,6 @@ public static boolean doObjectIterator(VirtualFrame frame, LocalAccessor output,
18741874
@Bind BytecodeNode bytecode) {
18751875
if (!iterator.hasNext()) {
18761876
iterator.setExhausted();
1877-
output.setObject(bytecode, frame, null);
18781877
return false;
18791878
}
18801879
Object value = iterator.next();
@@ -1917,7 +1916,6 @@ public static boolean doIterator(VirtualFrame frame, LocalAccessor output, Objec
19171916
output.setObject(bytecode, frame, value);
19181917
return true;
19191918
} catch (IteratorExhausted e) {
1920-
output.setObject(bytecode, frame, null);
19211919
return false;
19221920
}
19231921
}
@@ -3920,38 +3918,6 @@ public static Object perform(
39203918
}
39213919
}
39223920

3923-
@Operation(storeBytecodeIndex = false)
3924-
@ConstantOperand(type = LocalAccessor.class)
3925-
public static final class TeeLocal {
3926-
@Specialization
3927-
public static int doInt(VirtualFrame frame, LocalAccessor local, int value,
3928-
@Bind BytecodeNode bytecode) {
3929-
local.setInt(bytecode, frame, value);
3930-
return value;
3931-
}
3932-
3933-
@Specialization
3934-
public static double doDouble(VirtualFrame frame, LocalAccessor local, double value,
3935-
@Bind BytecodeNode bytecode) {
3936-
local.setDouble(bytecode, frame, value);
3937-
return value;
3938-
}
3939-
3940-
@Specialization
3941-
public static long doLong(VirtualFrame frame, LocalAccessor local, long value,
3942-
@Bind BytecodeNode bytecode) {
3943-
local.setLong(bytecode, frame, value);
3944-
return value;
3945-
}
3946-
3947-
@Specialization(replaces = {"doInt", "doDouble", "doLong"})
3948-
public static Object doObject(VirtualFrame frame, LocalAccessor local, Object value,
3949-
@Bind BytecodeNode bytecode) {
3950-
local.setObject(bytecode, frame, value);
3951-
return value;
3952-
}
3953-
}
3954-
39553921
@Operation(storeBytecodeIndex = true)
39563922
public static final class GetLen {
39573923
@Specialization

0 commit comments

Comments
 (0)