[GR-64741] optimize unstar using Variadic annotations

PullRequest: graalpython/3877

Author: Michal Medvecky

graalpython/com.oracle.graal.python.test/src/tests/test_functions.py

@@ -1,4 +1,4 @@
-# Copyright (c) 2018, 2024, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2018, 2025, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 #
 # The Universal Permissive License (UPL), Version 1.0
@@ -295,3 +295,30 @@ def foo():
     assert foo.__doc__ == 'my doc'
     foo2 = type(foo)(foo.__code__, foo.__globals__, foo.__name__, foo.__defaults__, foo.__closure__)
     assert foo2.__doc__ == 'my doc'
+
+def test_args_kwargs_eval_order():
+    l = []
+    def a1():
+        nonlocal l
+        l.append(101)
+        return 1
+    def a2():
+        nonlocal l
+        l.append(102)
+        return 1
+    def k1():
+        nonlocal l
+        l.append(201)
+        return 1
+    def k2():
+        nonlocal l
+        l.append(202)
+        return 1
+    def k3():
+        nonlocal l
+        l.append(203)
+        return 1
+    def f(*args, **kwargs):
+        pass
+    f(a1(), a2(), k1=k1(), k2=k2(), k3=k3())
+    assert l == [101, 102, 201, 202, 203]

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

@@ -1094,8 +1094,6 @@ public BytecodeDSLCompilerResult visit(ExprTy.ListComp node) {
         return buildComprehensionCodeUnit(node, node.generators, "<listcomp>",
                         (statementCompiler) -> {
                             statementCompiler.b.beginMakeList();
-                            // TODO: GR-64741 (do not collect to array manually)
-                            statementCompiler.b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY);
                             statementCompiler.b.endMakeList();
                         },
                         (statementCompiler, collection) -> {
@@ -1127,7 +1125,6 @@ public BytecodeDSLCompilerResult visit(ExprTy.SetComp node) {
         return buildComprehensionCodeUnit(node, node.generators, "<setcomp>",
                         (statementCompiler) -> {
                             statementCompiler.b.beginMakeSet();
-                            statementCompiler.b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY);
                             statementCompiler.b.endMakeSet();
                         },
                         (statementCompiler, collection) -> {
@@ -1788,7 +1785,9 @@ private void emitCall(ExprTy func, ExprTy[] args, KeywordTy[] keywords) {
                     b.emitLoadLocal(function);
                     b.endBlock();
 
+                    b.beginCollectToObjectArray();
                     emitUnstar(() -> b.emitLoadLocal(receiver), args);
+                    b.endCollectToObjectArray();
                     emitKeywords(keywords, function);
                 } else {
                     assert len(keywords) == 0;
@@ -1809,7 +1808,9 @@ private void emitCall(ExprTy func, ExprTy[] args, KeywordTy[] keywords) {
                     b.emitLoadLocal(function);
                     b.endBlock();
 
+                    b.beginCollectToObjectArray();
                     emitUnstar(args);
+                    b.endCollectToObjectArray();
                     emitKeywords(keywords, function);
                 } else {
                     assert len(keywords) == 0;
@@ -2011,12 +2012,9 @@ private void createConstant(ConstantValue value) {
                     break;
                 case TUPLE:
                     b.beginMakeTuple();
-                    // TODO: GR-64741 (do not collect to array manually)
-                    b.beginCollectToObjectArray();
                     for (ConstantValue cv : value.getTupleElements()) {
                         createConstant(cv);
                     }
-                    b.endCollectToObjectArray();
                     b.endMakeTuple();
                     break;
                 case FROZENSET:
@@ -2309,97 +2307,69 @@ private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args) {
         private void emitUnstar(Runnable initialElementsProducer, ExprTy[] args, Runnable finalElementsProducer) {
             boolean noExtraElements = initialElementsProducer == null && finalElementsProducer == null;
             if (noExtraElements && len(args) == 0) {
-                b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY);
+                /**
+                 * We don't need to emit anything for an empty array.
+                 */
             } else if (noExtraElements && len(args) == 1 && args[0] instanceof ExprTy.Starred) {
                 // Optimization for single starred argument: we can just upack it. For generic
                 // algorithm see the next branch
-                b.beginUnpackStarred();
+                b.beginUnpackStarredVariadic();
                 ((ExprTy.Starred) args[0]).value.accept(this);
-                b.endUnpackStarred();
+                b.endUnpackStarredVariadic();
             } else if (anyIsStarred(args)) {
                 /**
-                 * We emit one or more arrays and concatenate them using Unstar. Each array
-                 * corresponds to a contiguous sequence of arguments or the result of unpacking a
-                 * single starred argument.
+                 * We emit one or more arrays. These are not concatenated directly, but rather
+                 * expect that the caller is receiving them into @Variadic annotated argument, as that handles
+                 * the concatenation. Each array corresponds to a contiguous sequence of arguments or the result
+                 * of unpacking a single starred argument.
                  *
                  * For example, for the argument list a, b, *c, d, e, *f, g we would emit:
                  *
                  * @formatter:off
-                 * Unstar(
-                 *   CollectToObjectArray(a, b),
-                 *   UnpackStarred(c),
-                 *   CollectToObjectArray(d, e),
-                 *   UnpackStarred(f),
-                 *   CollectToObjectArray(g)
-                 * )
+                 *   a,
+                 *   b,
+                 *   UnpackStarredVariadic(c),
+                 *   d,
+                 *   e,
+                 *   UnpackStarredVariadic(f),
+                 *   g
                  * @formatter:on
+                 *
+                 * CollectObjectToArray is no longer necessary, as the UnpackStarredVariadic return @Variadic.
                  */
-                b.beginUnstar();
-                boolean inVariadic = false;
-                int numOperands = 0;
-
                 if (initialElementsProducer != null) {
-                    b.beginCollectToObjectArray();
                     initialElementsProducer.run();
-                    inVariadic = true;
                 }
 
                 for (int i = 0; i < args.length; i++) {
                     if (args[i] instanceof ExprTy.Starred) {
-                        if (inVariadic) {
-                            b.endCollectToObjectArray();
-                            inVariadic = false;
-                            numOperands++;
-                        }
-
-                        b.beginUnpackStarred();
+                        b.beginUnpackStarredVariadic();
                         ((ExprTy.Starred) args[i]).value.accept(this);
-                        b.endUnpackStarred();
-                        numOperands++;
+                        b.endUnpackStarredVariadic();
                     } else {
-                        if (!inVariadic) {
-                            b.beginCollectToObjectArray();
-                            inVariadic = true;
-                        }
-
                         args[i].accept(this);
                     }
                 }
 
                 if (finalElementsProducer != null) {
-                    if (!inVariadic) {
-                        b.beginCollectToObjectArray();
-                        inVariadic = true;
-                    }
                     finalElementsProducer.run();
                 }
-
-                if (inVariadic) {
-                    b.endCollectToObjectArray();
-                    numOperands++;
-                }
-
-                b.endUnstar(numOperands);
             } else {
-                b.beginCollectToObjectArray();
                 if (initialElementsProducer != null) {
                     initialElementsProducer.run();
                 }
                 visitSequence(args);
                 if (finalElementsProducer != null) {
                     finalElementsProducer.run();
                 }
-                b.endCollectToObjectArray();
             }
         }
 
         @Override
         public Void visit(ExprTy.Set node) {
             boolean newStatement = beginSourceSection(node, b);
             b.beginMakeSet();
-            if (len(node.elements) == 0) {
-                b.emitLoadConstant(PythonUtils.EMPTY_OBJECT_ARRAY);
-            } else {
+            if (len(node.elements) != 0) {
                 emitUnstar(node.elements);
             }
             b.endMakeSet();
@@ -2814,12 +2784,9 @@ public void visitTypeParams(TypeParamTy[] typeParams) {
             } else {
                 b.beginMakeTuple();
             }
-            // TODO: GR-64741 (do not collect to array manually)
-            b.beginCollectToObjectArray();
             for (TypeParamTy typeParam : typeParams) {
                 typeParam.accept(this);
             }
-            b.endCollectToObjectArray();
             if (useList) {
                 b.endMakeList();
             } else {
@@ -3348,10 +3315,12 @@ private void emitBuildClass(BytecodeDSLCodeUnit body, ClassDef node) {
             }
 
             // positional args
+            b.beginCollectToObjectArray();
             emitUnstar(() -> {
                 emitMakeFunction(body, node, node.name, null, null);
                 emitPythonConstant(toTruffleStringUncached(node.name), b);
             }, node.bases, finalElements);
+            b.endCollectToObjectArray();
 
             // keyword args
             validateKeywords(node.keywords);

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

@@ -1493,7 +1493,7 @@ public static Object perform(VirtualFrame frame,
     @Operation
     public static final class MakeList {
         @Specialization
-        public static PList perform(Object[] elements,
+        public static PList perform(@Variadic Object[] elements,
                         @Bind PBytecodeDSLRootNode rootNode) {
             return PFactory.createList(rootNode.getLanguage(), elements);
         }
@@ -1503,14 +1503,14 @@ public static PList perform(Object[] elements,
     @ImportStatic({PBytecodeDSLRootNode.class})
     public static final class MakeSet {
         @Specialization(guards = "elements.length == 0")
-        public static PSet doEmpty(VirtualFrame frame, Object[] elements,
+        public static PSet doEmpty(VirtualFrame frame, @Variadic Object[] elements,
                         @Bind PBytecodeDSLRootNode rootNode) {
             // creates set backed by empty HashingStorage
             return PFactory.createSet(rootNode.getLanguage());
         }
 
         @Specialization(guards = "elements.length != 0")
-        public static PSet doNonEmpty(VirtualFrame frame, Object[] elements,
+        public static PSet doNonEmpty(VirtualFrame frame, @Variadic Object[] elements,
                         @Bind PBytecodeDSLRootNode rootNode,
                         @Bind Node inliningTarget,
                         @Cached MakeSetStorageNode makeSetStorageNode) {
@@ -1539,7 +1539,7 @@ public static PFrozenSet doNonEmpty(VirtualFrame frame, @Variadic Object[] eleme
     @Operation
     public static final class MakeTuple {
         @Specialization
-        public static Object perform(Object[] elements,
+        public static Object perform(@Variadic Object[] elements,
                         @Bind PBytecodeDSLRootNode rootNode) {
             return PFactory.createTuple(rootNode.getLanguage(), elements);
         }
@@ -2510,32 +2510,6 @@ public static PCell[] doMakeCellArray(@Variadic Object[] cells) {
         }
     }
 
-    /**
-     * Flattens an array of arrays. Used for splatting Starred expressions.
-     */
-    @Operation
-    @ConstantOperand(type = int.class, specifyAtEnd = true)
-    public static final class Unstar {
-        @Specialization(guards = "length != 1")
-        public static Object[] perform(@Variadic Object[] values, int length) {
-            // if len <= 1, we should emit load constant "empty array", or emit just unpack starred
-            assert length > 1;
-            CompilerAsserts.partialEvaluationConstant(length);
-            int totalLength = 0;
-            for (int i = 0; i < length; i++) {
-                totalLength += ((Object[]) values[i]).length;
-            }
-            Object[] result = new Object[totalLength];
-            int idx = 0;
-            for (int i = 0; i < length; i++) {
-                int nl = ((Object[]) values[i]).length;
-                System.arraycopy(values[i], 0, result, idx, nl);
-                idx += nl;
-            }
-            return result;
-        }
-    }
-
     @Operation
     @ConstantOperand(type = LocalAccessor.class)
     public static final class KwargsMerge {
@@ -2565,8 +2539,9 @@ public static PDict doMerge(VirtualFrame frame,
     }
 
     @Operation
+    @Variadic
     @ImportStatic({PGuards.class})
-    public static final class UnpackStarred {
+    public static final class UnpackStarredVariadic {
         public static boolean isListOrTuple(PSequence obj, Node inliningTarget, InlinedConditionProfile isListProfile) {
             return isListProfile.profile(inliningTarget, PGuards.isBuiltinList(obj)) || PGuards.isBuiltinTuple(obj);
         }
@@ -2592,7 +2567,7 @@ public static Object[] doUnpackIterable(VirtualFrame virtualFrame, Object collec
                         @Cached PyObjectGetIter getIter,
                         @Cached PyIterNextNode getNextNode,
                         @Cached IsBuiltinObjectProfile notIterableProfile,
-                        @Exclusive @Cached PRaiseNode raiseNode) {
+                        @Cached PRaiseNode raiseNode) {
 
             Object iterator;
             try {

mx.graalpython/mx_graalpython.py

@@ -2072,28 +2072,9 @@ def hardcoded_ver_is_behind_major_minor(m):
             return f"Hardcoded version in `{m.group().strip()}` should have {GRAAL_VERSION_MAJ_MIN} as <major>.<minor> version."
 
     files_with_versions = {
-        "graalpython/graalpy-maven-plugin/pom.xml": {
-            r"^  <version>(\d+\.\d+)(?:\.\d+)*</version>" : hardcoded_ver_is_behind_major_minor,
-            r'<graalpy.version>(\d+\.\d+)(?:\.\d+)*</graalpy.version>' : hardcoded_ver_is_behind_major_minor
-        },
         "graalpython/com.oracle.graal.python.test.integration/pom.xml": {
             r'<com.oracle.graal.python.test.polyglot.version>(\d+\.\d+)(?:\.\d+)*' : hardcoded_ver_is_behind_major_minor,
-        },
-        "graalpython/graalpy-archetype-polyglot-app/pom.xml": {
-            r"^  <version>(\d+\.\d+)(?:\.\d+)*</version>" : hardcoded_ver_is_behind_major_minor,
-        },
-        "graalpython/graalpy-jbang/examples/hello.java": {
-            r"//DEPS org.graalvm.python:jbang[^:]*:\${env.GRAALPY_VERSION:(\d+\.\d+)(?:\.\d+)*" : hardcoded_ver_is_behind_major_minor,
-        },
-        "graalpython/graalpy-jbang/templates/graalpy-template_local_repo.java.qute": {
-            r"//DEPS org.graalvm.python:jbang[^:]*:\${env.GRAALPY_VERSION:(\d+\.\d+)(?:\.\d+)*" : hardcoded_ver_is_behind_major_minor,
-        },
-        "graalpython/graalpy-jbang/templates/graalpy-template.java.qute": {
-            r"//DEPS org.graalvm.python:jbang[^:]*:\${env.GRAALPY_VERSION:(\d+\.\d+)(?:\.\d+)*" : hardcoded_ver_is_behind_major_minor,
-        },
-        "graalpython/graalpy-archetype-polyglot-app/src/main/resources/archetype-resources/pom.xml": {
-            r'<graalpy.version>(\d+\.\d+)(?:\.\d+)*</graalpy.version>' : hardcoded_ver_is_behind_major_minor,
-        },
+        }
     }
     replacements = set()
     for path, patterns in files_with_versions.items():