GH-105848: Simplify the arrangement of `CALL`'s stack #107788

brandtbucher · 2023-08-08T20:01:41Z

Instead of supporting both [callable, self, args...] and [NULL, callable, args...], change CALL's stack to be [callable, self_or_null, args...]. This always puts the desired callable at the same location on the stack, simplifying (or removing) the resulting shuffles and adjustments necessary for the shared code paths that follow.

This also changes CALL_FUNCTION_EX, DICT_MERGE, LOAD_ATTR, LOAD_GLOBAL, LOAD_SUPER_ATTR to support the new stack layout. In some cases, I've also changed the names of the stack items for consistency within a family.

As a side-effect of the new layout, LOAD_ATTR_PROPERTY and LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN (which are both implemented as inlined calls) don't support pushing an additional item to the stack for a following CALL (oparg & 1) anymore. I think this is fine, since it seems rare to call a "method" that's looked up in this manner.

Issue: Clean up the CALL sequence #105848

gvanrossum

I think this can go in just like this.

gvanrossum · 2023-08-08T22:42:40Z

Python/executor_cases.c.h

-            if (oparg & 1) { stack_pointer[-1 - (oparg & 1 ? 1 : 0)] = null; }
-            stack_pointer[-1] = v;
+            stack_pointer[-1 - (oparg & 1 ? 1 : 0)] = res;
+            if (oparg & 1) { stack_pointer[-(oparg & 1 ? 1 : 0)] = null; }


Theoretically this could be slightly less performant than the original (not that it matters, and the C compiler might see right through this).

Python/flowgraph.c

gvanrossum · 2023-08-08T22:57:33Z

Python/bytecodes.c

@@ -1616,7 +1616,7 @@ dummy_func(
            PyObject *dict = PEEK(oparg + 1);  // update is still on the stack


Maybe we could make the DSL less of a lie, e.g. using

inst(DICT_MERGE, (dict, unused[oparg-1], update -- dict, unused[oparg-1])) {

?

(I guess the peeks in the error handling code suggest it's even more complicated.)

I figured the presence of the PEEK was an indicator that someone had tried that already. Maybe not, though... let me see!

Maybe it was an oversight. Maybe the code generator didn't use to handle this case.

I tried what I suggested and it seems to work. I didn't try to figure out what to do for the error handling (probably some more dummies below dict).

gvanrossum · 2023-08-08T23:00:29Z

Python/bytecodes.c

@@ -1644,7 +1644,7 @@ dummy_func(
            LOAD_SUPER_ATTR_METHOD,
        };

-        inst(LOAD_SUPER_ATTR, (unused/1, global_super, class, self -- res2 if (oparg & 1), res)) {
+        inst(LOAD_SUPER_ATTR, (unused/1, global_super, class, self -- attr, self_or_null if (oparg & 1))) {


self_or_null is unused, so replace with unused?

Putting unused here breaks things. Since this is named it's set to NULL in the generated code... if it's called unused, it just ignores it and now there's a garbage value there.

Oh sneaky. I think you found a hole in the code generator. If you want it to (conditionally) push NULL you should initialize it, not depend on the default initialization. IIRC the default initialization is there only to avoid C compiler warnings.

gvanrossum · 2023-08-08T23:00:45Z

Python/bytecodes.c

@@ -1705,39 +1705,38 @@ dummy_func(
            LOAD_SUPER_ATTR,
        };

-        inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- res2 if (oparg & 1), res)) {
+        inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- attr, self_or_null if (oparg & 1))) {


gvanrossum · 2023-08-08T23:03:46Z

Python/bytecodes.c

@@ -2795,9 +2792,9 @@ dummy_func(
        }

        inst(INSTRUMENTED_CALL, ( -- )) {
-            int is_meth = PEEK(oparg+2) != NULL;
+            int is_meth = PEEK(oparg + 1) != NULL;


gvanrossum · 2023-08-08T23:13:53Z

How about some buildbot runs?

bedevere-bot · 2023-08-08T23:19:12Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit fd6607f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot · 2023-08-08T23:19:12Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit fd6607f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

brandtbucher · 2023-08-09T00:11:55Z

Perf is neutral.

bedevere-bot · 2023-08-09T00:32:29Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit a5a2d8f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot · 2023-08-09T00:32:29Z

🤖 New build scheduled with the buildbot fleet by @brandtbucher for commit a5a2d8f 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

markshannon · 2023-08-09T09:03:03Z

No new buildbot failures.

markshannon

A bunch of minor style issues, but nothing blocking.

markshannon · 2023-08-09T09:06:43Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
-                callable = method;
+            if (self_or_null) {


This is perfectly correct, but could you make this consistent with the other NULL checks and use self_or_null != NULL to make it clear to the reader that this is a NULL check not a test of a boolean/int flag.

markshannon · 2023-08-09T09:07:08Z

Python/bytecodes.c

            int argcount = oparg;
-            if (is_meth) {
-                callable = method;
+            if (self_or_null) {


markshannon · 2023-08-09T09:07:18Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
-                callable = method;
+            if (self_or_null) {


markshannon · 2023-08-09T09:07:36Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
-                callable = method;
+            if (self_or_null) {


markshannon · 2023-08-09T09:07:49Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
-                callable = method;
+            if (self_or_null) {


markshannon · 2023-08-09T09:09:06Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
+            if (self_or_null) {


markshannon · 2023-08-09T09:09:25Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
+            if (self_or_null) {


almost the last one

markshannon · 2023-08-09T09:09:32Z

Python/bytecodes.c

            int total_args = oparg;
-            if (is_meth) {
+            if (self_or_null) {


the last one

Actually, you missed one. But don't worry, I got it. ;)

markshannon · 2023-08-09T09:10:45Z

Python/bytecodes.c

@@ -2712,7 +2704,7 @@ dummy_func(
            exc_info->exc_value = Py_NewRef(new_exc);
        }

-        inst(LOAD_ATTR_METHOD_WITH_VALUES, (unused/1, type_version/2, keys_version/2, descr/4, self -- res2 if (1), res)) {
+        inst(LOAD_ATTR_METHOD_WITH_VALUES, (unused/1, type_version/2, keys_version/2, descr/4, self -- attr, self_or_null if (1))) {


This isn't self_or_null, it's just self.

markshannon · 2023-08-09T09:18:53Z

Python/bytecodes.c

@@ -1705,39 +1702,38 @@ dummy_func(
            LOAD_SUPER_ATTR,
        };

-        inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- res2 if (oparg & 1), res)) {
+        inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- attr, unused if (oparg & 1))) {


Suggested change

inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- attr, unused if (oparg & 1))) {

inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- attr, unused if (0))) {

markshannon · 2023-08-09T09:35:26Z

Python/bytecodes.c

            DECREF_INPUTS();
        }

-        inst(LOAD_ATTR_PROPERTY, (unused/1, type_version/2, func_version/2, fget/4, owner -- unused if (oparg & 1), unused)) {
+        inst(LOAD_ATTR_PROPERTY, (unused/1, type_version/2, func_version/2, fget/4, owner -- unused, unused if (oparg & 1))) {


Suggested change

inst(LOAD_ATTR_PROPERTY, (unused/1, type_version/2, func_version/2, fget/4, owner -- unused, unused if (oparg & 1))) {

inst(LOAD_ATTR_PROPERTY, (unused/1, type_version/2, func_version/2, fget/4, owner -- unused, unused if (0))) {

brandtbucher added 5 commits August 5, 2023 08:45

Clean up the call sequence

80fa37a

blurb add

f565bc6

Fix doctest

b48c86d

Bump the magic number

fd6607f

DSL cleanup

a5a2d8f

brandtbucher added performance Performance or resource usage interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Aug 8, 2023

brandtbucher requested a review from gvanrossum August 8, 2023 20:01

brandtbucher self-assigned this Aug 8, 2023

brandtbucher requested review from markshannon and iritkatriel as code owners August 8, 2023 20:01

bedevere-bot mentioned this pull request Aug 8, 2023

Clean up the CALL sequence #105848

Closed

bedevere-bot added the awaiting core review label Aug 8, 2023

brandtbucher mentioned this pull request Aug 8, 2023

gh-106581: Split CALL_PY_EXACT_ARGS into uops #107760

Merged

4 tasks

brandtbucher requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners August 8, 2023 23:02

gvanrossum approved these changes Aug 8, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Aug 8, 2023

brandtbucher added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Aug 8, 2023

bedevere-bot removed 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Aug 8, 2023

gvanrossum mentioned this pull request Aug 8, 2023

Call design for Tier 2 (uops) interpreter #106581

Closed

gvanrossum approved these changes Aug 9, 2023

View reviewed changes

brandtbucher added 🔨 test-with-buildbots Test PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Aug 9, 2023

bedevere-bot removed 🔨 test-with-buildbots Test PR w/ buildbots; report in status section 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section labels Aug 9, 2023

markshannon reviewed Aug 9, 2023

View reviewed changes

markshannon mentioned this pull request Aug 9, 2023

Handle long tail of specialization for LOAD_ATTR and CALL with trampolines. faster-cpython/ideas#447

Open

More tweaks

47009af

brandtbucher enabled auto-merge (squash) August 9, 2023 17:54

brandtbucher merged commit a9caf9c into python:main Aug 9, 2023
22 checks passed

bedevere-bot removed the awaiting merge label Aug 9, 2023

brandtbucher mentioned this pull request Sep 12, 2023

GH-105848: Replace KW_NAMES + CALL with LOAD_CONST + CALL_KW #109300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GH-105848: Simplify the arrangement of `CALL`'s stack #107788

GH-105848: Simplify the arrangement of `CALL`'s stack #107788

brandtbucher commented Aug 8, 2023 •

edited by bedevere-bot

gvanrossum left a comment

gvanrossum Aug 8, 2023

gvanrossum Aug 8, 2023

brandtbucher Aug 8, 2023

gvanrossum Aug 8, 2023

gvanrossum Aug 8, 2023

brandtbucher Aug 9, 2023

gvanrossum Aug 9, 2023

gvanrossum Aug 8, 2023

gvanrossum Aug 8, 2023

gvanrossum commented Aug 8, 2023

bedevere-bot commented Aug 8, 2023

bedevere-bot commented Aug 8, 2023

brandtbucher commented Aug 9, 2023

bedevere-bot commented Aug 9, 2023

bedevere-bot commented Aug 9, 2023

markshannon commented Aug 9, 2023

markshannon left a comment

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

brandtbucher Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

markshannon Aug 9, 2023

		@@ -1616,7 +1616,7 @@ dummy_func(
		PyObject *dict = PEEK(oparg + 1); // update is still on the stack

	inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- attr, unused if (oparg & 1))) {
	inst(LOAD_SUPER_ATTR_ATTR, (unused/1, global_super, class, self -- attr, unused if (0))) {

	inst(LOAD_ATTR_PROPERTY, (unused/1, type_version/2, func_version/2, fget/4, owner -- unused, unused if (oparg & 1))) {
	inst(LOAD_ATTR_PROPERTY, (unused/1, type_version/2, func_version/2, fget/4, owner -- unused, unused if (0))) {

GH-105848: Simplify the arrangement of CALL's stack #107788

GH-105848: Simplify the arrangement of CALL's stack #107788

Conversation

brandtbucher commented Aug 8, 2023 • edited by bedevere-bot

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Aug 8, 2023

bedevere-bot commented Aug 8, 2023

bedevere-bot commented Aug 8, 2023

brandtbucher commented Aug 9, 2023

bedevere-bot commented Aug 9, 2023

bedevere-bot commented Aug 9, 2023

markshannon commented Aug 9, 2023

markshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GH-105848: Simplify the arrangement of `CALL`'s stack #107788

GH-105848: Simplify the arrangement of `CALL`'s stack #107788

brandtbucher commented Aug 8, 2023 •

edited by bedevere-bot