GH-105848: Simplify the arrangement of `CALL`'s stack by brandtbucher · Pull Request #107788 · python/cpython · GitHub

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

I think this can go in just like this.

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

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).

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.

Ditto.

Yes!

How about some buildbot runs?

🤖 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.

🤖 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.

Perf is neutral.

🤖 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.

🤖 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.

No new buildbot failures.

A bunch of minor style issues, but nothing blocking.

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.

ditto

likewise

and here

another

almost the last one

the last one

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

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

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))) {

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))) {

Summary: This reflects the changes in python/cpython#107788. The changes to the relevant `LOAD_` instructions are mostly covered by updating `LoadMethodResult` to have a non-default constructor in 3.14+ which normalizes the content. Reviewed By: alexmalyshev Differential Revision: D80667929 fbshipit-source-id: 2d62f1850056792695faf8d9ceea61a0b4b31253

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 iritkatriel and markshannon 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

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

-                          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; }

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

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

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

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

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

                       };
-                      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))) {

                       };
-                      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))) {

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

                       }
-                      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))) {

	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 by brandtbucher · Pull Request #107788 · python/cpython · GitHub

Uh oh!

Uh oh!

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

GH-105848: Simplify the arrangement of CALL's stack#107788brandtbucher merged 6 commits intopython:mainpython/cpython:mainfrom brandtbucher:callsbrandtbucher/cpython:callsCopy head branch name to clipboard

Conversation

brandtbucher commented Aug 8, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Aug 8, 2023

Uh oh!

bedevere-bot commented Aug 8, 2023

Uh oh!

bedevere-bot commented Aug 8, 2023

Uh oh!

brandtbucher commented Aug 9, 2023

Uh oh!

bedevere-bot commented Aug 9, 2023

Uh oh!

bedevere-bot commented Aug 9, 2023

Uh oh!

markshannon commented Aug 9, 2023

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

GH-105848: Simplify the arrangement of CALL's stack#107788
brandtbucher merged 6 commits into
python:mainfrom
brandtbucher:calls

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