From 5ac8f9a5b05a25af9df4a2d8f132a6b54ccf0e97 Mon Sep 17 00:00:00 2001 From: Matt D'Souza Date: Fri, 11 Oct 2024 16:01:10 -0400 Subject: [PATCH] Fix unbound local checks, remove unused/redundant operations --- .../bytecode_dsl/RootNodeCompiler.java | 81 ++++----- .../bytecode_dsl/PBytecodeDSLRootNode.java | 160 +++++------------- 2 files changed, 85 insertions(+), 156 deletions(-) diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/bytecode_dsl/RootNodeCompiler.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/bytecode_dsl/RootNodeCompiler.java index 2942f5b4e9..9f16f990d2 100644 --- a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/bytecode_dsl/RootNodeCompiler.java +++ b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/compiler/bytecode_dsl/RootNodeCompiler.java @@ -19,6 +19,7 @@ import com.oracle.graal.python.builtins.objects.ellipsis.PEllipsis; import com.oracle.graal.python.builtins.objects.function.PArguments; import com.oracle.graal.python.builtins.objects.function.PKeyword; +import com.oracle.graal.python.builtins.objects.type.TypeFlags; import com.oracle.graal.python.compiler.CompilationScope; import com.oracle.graal.python.compiler.Compiler; import com.oracle.graal.python.compiler.Compiler.ConstantCollection; @@ -845,7 +846,11 @@ private BytecodeDSLCompilerResult buildComprehensionCodeUnit(SSTNode node, Compr @Override public BytecodeDSLCompilerResult visit(ExprTy.ListComp node) { return buildComprehensionCodeUnit(node, node.generators, "", - (statementCompiler) -> statementCompiler.b.emitMakeEmptyList(), + (statementCompiler) -> { + statementCompiler.b.beginMakeList(); + statementCompiler.b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY); + statementCompiler.b.endMakeList(); + }, (statementCompiler, collection) -> { statementCompiler.b.beginListAppend(); statementCompiler.b.emitLoadLocal(collection); @@ -857,7 +862,10 @@ public BytecodeDSLCompilerResult visit(ExprTy.ListComp node) { @Override public BytecodeDSLCompilerResult visit(ExprTy.DictComp node) { return buildComprehensionCodeUnit(node, node.generators, "", - (statementCompiler) -> statementCompiler.b.emitMakeEmptyDict(), + (statementCompiler) -> { + statementCompiler.b.beginMakeDict(0); + statementCompiler.b.endMakeDict(); + }, (statementCompiler, collection) -> { statementCompiler.b.beginSetDictItem(); statementCompiler.b.emitLoadLocal(collection); @@ -870,7 +878,11 @@ public BytecodeDSLCompilerResult visit(ExprTy.DictComp node) { @Override public BytecodeDSLCompilerResult visit(ExprTy.SetComp node) { return buildComprehensionCodeUnit(node, node.generators, "", - (statementCompiler) -> statementCompiler.b.emitMakeEmptySet(), + (statementCompiler) -> { + statementCompiler.b.beginMakeSet(); + statementCompiler.b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY); + statementCompiler.b.endMakeSet(); + }, (statementCompiler, collection) -> { statementCompiler.b.beginSetAdd(); statementCompiler.b.emitLoadLocal(collection); @@ -968,21 +980,14 @@ private void emitNameFastOperation(String mangled, NameOperation op, Builder b) BytecodeLocal local = locals.get(mangled); switch (op) { case Read: - b.beginCheckUnboundLocal(varnames.get(mangled)); - b.emitLoadLocal(local); - b.endCheckUnboundLocal(); - break; - case Delete: b.beginBlock(); - b.beginCheckUnboundLocal(varnames.get(mangled)); + b.emitCheckUnboundLocal(local, varnames.get(mangled)); b.emitLoadLocal(local); - b.endCheckUnboundLocal(); - - b.beginStoreLocal(local); - b.emitLoadNull(); - b.endStoreLocal(); b.endBlock(); break; + case Delete: + b.emitDeleteLocal(local, varnames.get(mangled)); + break; case BeginWrite: if (local == null) { throw new NullPointerException("local " + mangled + " not defined"); @@ -1142,7 +1147,7 @@ public void setUpFrame(ArgumentsTy args, Builder b) { List toClear = new ArrayList<>(); b.beginStoreRange(cellVariableLocals); - b.beginMakeVariadic(); + b.beginCollectToObjectArray(); for (int i = 0; i < cellVariableLocals.length; i++) { b.beginCreateCell(); if (scope.getUseOfName(cellVariables[i]).contains(DefUse.DefParam)) { @@ -1159,7 +1164,7 @@ public void setUpFrame(ArgumentsTy args, Builder b) { } b.endCreateCell(); } - b.endMakeVariadic(); + b.endCollectToObjectArray(); b.endStoreRange(); for (BytecodeLocal local : toClear) { @@ -1722,11 +1727,11 @@ private void createConstant(ConstantValue value) { break; case TUPLE: b.beginMakeTuple(); - b.beginMakeVariadic(); + b.beginCollectToObjectArray(); for (ConstantValue cv : value.getTupleElements()) { createConstant(cv); } - b.endMakeVariadic(); + b.endCollectToObjectArray(); b.endMakeTuple(); break; case FROZENSET: @@ -1767,7 +1772,8 @@ public Void visit(ExprTy.Dict node) { boolean newStatement = beginSourceSection(node, b); if (len(node.keys) == 0) { - b.emitMakeEmptyDict(); + b.beginMakeDict(0); + b.endMakeDict(); } else { b.beginMakeDict(node.keys.length); for (int i = 0; i < node.keys.length; i++) { @@ -2027,11 +2033,11 @@ private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args) { * * @formatter:off * Unstar( - * MakeVariadic(a, b), + * CollectToObjectArray(a, b), * UnpackStarred(c), - * MakeVariadic(d, e), + * CollectToObjectArray(d, e), * UnpackStarred(f), - * MakeVariadic(g) + * CollectToObjectArray(g) * ) * @formatter:on */ @@ -2040,7 +2046,7 @@ private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args) { int numOperands = 0; if (initialElementsProducer != null) { - b.beginMakeVariadic(); + b.beginCollectToObjectArray(); initialElementsProducer.run(); inVariadic = true; } @@ -2048,7 +2054,7 @@ private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args) { for (int i = 0; i < args.length; i++) { if (args[i] instanceof ExprTy.Starred) { if (inVariadic) { - b.endMakeVariadic(); + b.endCollectToObjectArray(); inVariadic = false; numOperands++; } @@ -2059,7 +2065,7 @@ private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args) { numOperands++; } else { if (!inVariadic) { - b.beginMakeVariadic(); + b.beginCollectToObjectArray(); inVariadic = true; } @@ -2068,33 +2074,31 @@ private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args) { } if (inVariadic) { - b.endMakeVariadic(); + b.endCollectToObjectArray(); numOperands++; } b.endUnstar(numOperands); } else { - b.beginMakeVariadic(); + b.beginCollectToObjectArray(); if (initialElementsProducer != null) { initialElementsProducer.run(); } visitSequence(args); - b.endMakeVariadic(); + b.endCollectToObjectArray(); } } @Override public Void visit(ExprTy.Set node) { boolean newStatement = beginSourceSection(node, b); - + b.beginMakeSet(); if (len(node.elements) == 0) { - b.emitMakeEmptySet(); + b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY); } else { - b.beginMakeSet(); emitUnstar(node.elements); - b.endMakeSet(); } - + b.endMakeSet(); endSourceSection(b, newStatement); return null; } @@ -2943,7 +2947,8 @@ private void emitKeywordGroup(KeywordGroup group, boolean copy, BytecodeLocal fu if (copy) { b.beginKwargsMerge(function); - b.emitMakeEmptyDict(); + b.beginMakeDict(0); + b.endMakeDict(); splatKeywords.expr.accept(this); b.endKwargsMerge(); } else { @@ -3229,11 +3234,11 @@ private void emitMakeFunction(SSTNode node, String name, ArgumentsTy args, List< if (args == null || len(args.defaults) == 0) { b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY); } else { - b.beginMakeVariadic(); + b.beginCollectToObjectArray(); for (int i = 0; i < args.defaults.length; i++) { args.defaults[i].accept(this); } - b.endMakeVariadic(); + b.endCollectToObjectArray(); } boolean hasKeywords = false; @@ -3822,9 +3827,9 @@ private void doVisitPattern(PatternTy.MatchSequence node, PatternContext pc) { b.beginPrimitiveBoolAnd(); - b.beginIsSequence(); + b.beginCheckTypeFlags(TypeFlags.SEQUENCE); b.emitLoadLocal(pc.subject); - b.endIsSequence(); + b.endCheckTypeFlags(); if (star < 0) { // No star: len(subject) == size diff --git a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode_dsl/PBytecodeDSLRootNode.java b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode_dsl/PBytecodeDSLRootNode.java index 33c774a51e..6bbae56f71 100644 --- a/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode_dsl/PBytecodeDSLRootNode.java +++ b/graalpython/com.oracle.graal.python/src/com/oracle/graal/python/nodes/bytecode_dsl/PBytecodeDSLRootNode.java @@ -127,7 +127,6 @@ import com.oracle.graal.python.nodes.expression.BinaryArithmetic.BitAndNode; import com.oracle.graal.python.nodes.expression.BinaryArithmetic.BitOrNode; import com.oracle.graal.python.nodes.expression.BinaryArithmetic.BitXorNode; -import com.oracle.graal.python.nodes.expression.BinaryArithmetic.DivModNode; import com.oracle.graal.python.nodes.expression.BinaryArithmetic.FloorDivNode; import com.oracle.graal.python.nodes.expression.BinaryArithmetic.LShiftNode; import com.oracle.graal.python.nodes.expression.BinaryArithmetic.MatMulNode; @@ -254,7 +253,6 @@ @OperationProxy(BitXorNode.class) @OperationProxy(MatMulNode.class) @OperationProxy(PowNode.class) -@OperationProxy(DivModNode.class) @OperationProxy(PosNode.class) @OperationProxy(NegNode.class) @OperationProxy(InvertNode.class) @@ -269,8 +267,7 @@ @OperationProxy(value = SetNodes.AddNode.class, name = "SetAdd") @ShortCircuitOperation(name = "BoolAnd", booleanConverter = PBytecodeDSLRootNode.Yes.class, operator = Operator.AND_RETURN_VALUE) @ShortCircuitOperation(name = "BoolOr", booleanConverter = PBytecodeDSLRootNode.Yes.class, operator = Operator.OR_RETURN_VALUE) -@ShortCircuitOperation(name = "PrimitiveBoolAnd", booleanConverter = PBytecodeDSLRootNode.BooleanIdentity.class, operator = Operator.AND_RETURN_CONVERTED) -@ShortCircuitOperation(name = "PrimitiveBoolOr", booleanConverter = PBytecodeDSLRootNode.BooleanIdentity.class, operator = Operator.OR_RETURN_CONVERTED) +@ShortCircuitOperation(name = "PrimitiveBoolAnd", operator = Operator.AND_RETURN_VALUE) @SuppressWarnings("unused") public abstract class PBytecodeDSLRootNode extends PRootNode implements BytecodeRootNode { public static final int EXPLODE_LOOP_THRESHOLD = 32; @@ -894,14 +891,6 @@ public Object readSelf(Frame frame) { } } - @Operation - public static final class BooleanIdentity { - @Specialization - public static boolean doBoolean(boolean b) { - return b; - } - } - @Operation public static final class ArrayIndex { @Specialization @@ -945,22 +934,19 @@ public static boolean perform(VirtualFrame frame, Object o, } } + /** + * Some operations take a single Object[] operand (e.g., {@link MakeTuple}). To pass a + * fixed-length sequence of elements to these operands (e.g., to instantiate a constant tuple) + * we need to first collect the values into an Object[]. + */ @Operation - public static final class MakeVariadic { + public static final class CollectToObjectArray { @Specialization public static Object[] perform(@Variadic Object[] values) { return values; } } - @Operation - public static final class GetVariableArguments { - @Specialization - public static Object[] perform(VirtualFrame frame) { - return PArguments.getVariableArguments(frame); - } - } - @Operation @ConstantOperand(type = TruffleString.class) public static final class WriteName { @@ -1002,19 +988,6 @@ public static boolean hasLocals(VirtualFrame frame) { } } - @Operation - public static final class LoadArgumentOptional { - @Specialization - public static Object perform(VirtualFrame frame, int argumentIndex) { - Object[] arguments = frame.getArguments(); - if (arguments.length > argumentIndex) { - return arguments[argumentIndex]; - } else { - return null; - } - } - } - @Operation public static final class LoadVariableArguments { @Specialization @@ -1967,15 +1940,6 @@ public static PSet performRegular(VirtualFrame frame, Object[] elements, } } - @Operation - public static final class MakeEmptySet { - @Specialization - public static PSet perform(VirtualFrame frame, - @Bind PBytecodeDSLRootNode rootNode) { - return rootNode.factory.createSet(); - } - } - @Operation @ConstantOperand(type = int.class) public static final class MakeFrozenSet { @@ -2017,14 +1981,6 @@ private static HashingStorage doRegular(VirtualFrame frame, Node inliningTarget, } - @Operation - public static final class MakeEmptyList { - @Specialization - public static PList perform(@Bind PBytecodeDSLRootNode rootNode) { - return rootNode.factory.createList(); - } - } - @Operation public static final class MakeTuple { @Specialization @@ -2251,14 +2207,6 @@ public static PDict perform(VirtualFrame frame, } - @Operation - public static final class MakeEmptyDict { - @Specialization - public static PDict perform(@Bind PBytecodeDSLRootNode rootNode) { - return rootNode.factory.createDict(); - } - } - @Operation public static final class SetDictItem { @Specialization @@ -3004,22 +2952,6 @@ public static void doNothing(@SuppressWarnings("unused") Object ex) { } } - @Operation - public static final class GetExceptionForReraise { - @Specialization - @InliningCutoff - public static PException doPException(PException ex, - @Bind PBytecodeDSLRootNode rootNode) { - return ex.getExceptionForReraise(!rootNode.isInternal()); - } - - @Fallback - @InliningCutoff - public static Object doNothing(@SuppressWarnings("unused") Object ex) { - return ex; - } - } - @Operation public static final class AssertFailed { @Specialization @@ -3708,22 +3640,6 @@ private static void doRegular(Object[] strings, int length, TruffleStringBuilder } } - @Operation - public static final class ListExtend { - @Specialization - public static void listExtend(VirtualFrame frame, PList list, Object obj, - @Bind Node inliningTarget, - @Cached IteratorNodes.GetLength lenNode, - @Cached("createExtend()") SequenceStorageNodes.ExtendNode extendNode) { - ListExtendNode.extendSequence(frame, list, obj, inliningTarget, lenNode, extendNode); - } - - @NeverDefault - public static SequenceStorageNodes.ExtendNode createExtend() { - return SequenceStorageNodes.ExtendNode.create(ListGeneralizationNode.SUPPLIER); - } - } - @Operation @ConstantOperand(type = LocalAccessor.class) public static final class TeeLocal { @@ -3756,14 +3672,6 @@ public static Object doObject(VirtualFrame frame, LocalAccessor local, Object va } } - @Operation - public static final class NonNull { - @Specialization - public static boolean doObject(Object value) { - return value != null; - } - } - @Operation public static final class GetLen { @Specialization @@ -3775,20 +3683,12 @@ public static int doObject(VirtualFrame frame, Object value, } @Operation - public static final class IsSequence { + @ConstantOperand(type = long.class) + public static final class CheckTypeFlags { @Specialization - public static boolean doObject(Object value, + public static boolean doObject(long typeFlags, Object value, @Cached GetTPFlagsNode getTPFlagsNode) { - return (getTPFlagsNode.execute(value) & TypeFlags.SEQUENCE) != 0; - } - } - - @Operation - public static final class IsMapping { - @Specialization - public static boolean doObject(Object value, - @Cached GetTPFlagsNode getTPFlagsNode) { - return (getTPFlagsNode.execute(value) & TypeFlags.MAPPING) != 0; + return (getTPFlagsNode.execute(value) & typeFlags) != 0; } } @@ -4053,22 +3953,46 @@ private static void handleException(VirtualFrame frame, PException e, Node inlin } @Operation + @ConstantOperand(type = LocalAccessor.class) @ConstantOperand(type = int.class) public static final class CheckUnboundLocal { @Specialization - public static Object doObject(int index, Object localValue, + public static void doObject(VirtualFrame frame, LocalAccessor accessor, int index, @Bind PBytecodeDSLRootNode rootNode, + @Bind BytecodeNode bytecodeNode, @Bind Node inliningTarget, - @Cached PRaiseNode.Lazy raiseNode) { - if (localValue == null) { - CompilerDirectives.transferToInterpreterAndInvalidate(); - TruffleString localName = rootNode.getCodeUnit().varnames[index]; - throw raiseNode.get(inliningTarget).raise(PythonBuiltinClassType.UnboundLocalError, ErrorMessages.LOCAL_VAR_REFERENCED_BEFORE_ASSIGMENT, localName); + @Cached InlinedBranchProfile localUnboundProfile) { + if (accessor.isCleared(bytecodeNode, frame)) { + localUnboundProfile.enter(inliningTarget); + throw raiseUnbound(rootNode, inliningTarget, index); } - return localValue; } } + @Operation + @ConstantOperand(type = LocalAccessor.class) + @ConstantOperand(type = int.class) + public static final class DeleteLocal { + @Specialization + public static void doObject(VirtualFrame frame, LocalAccessor accessor, int index, + @Bind PBytecodeDSLRootNode rootNode, + @Bind BytecodeNode bytecodeNode, + @Bind Node inliningTarget, + @Cached InlinedBranchProfile localUnboundProfile) { + if (accessor.isCleared(bytecodeNode, frame)) { + localUnboundProfile.enter(inliningTarget); + throw raiseUnbound(rootNode, inliningTarget, index); + } + accessor.clear(bytecodeNode, frame); + } + } + + @TruffleBoundary + private static PException raiseUnbound(PBytecodeDSLRootNode rootNode, Node inliningTarget, int index) { + TruffleString localName = rootNode.getCodeUnit().varnames[index]; + throw PRaiseNode.raiseUncached(inliningTarget, PythonBuiltinClassType.UnboundLocalError, ErrorMessages.LOCAL_VAR_REFERENCED_BEFORE_ASSIGMENT, localName); + } + @Operation public static final class RaiseNotImplementedError { @Specialization