GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-131798: JIT: Split `CALL_TYPE_1` into several uops by tomasr8 · Pull Request #132419 · python/cpython

Conversation

@tomasr8

@tomasr8 tomasr8 commented Apr 11, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This mostly works now, though the there are now two unused stackref variables in executor_cases.c.h (null and callable) and I'm not sure how to convince the cases generator to remove them. I tried:

  • using DEAD(x) in the guard uops - does not work because the stack variable also appears in the output of the uop. I also can't easily pop them off the stack since neither null or callable is TOS.
  • removing DEAD(x) from _CALL_TYPE_1: not possible, apparently this is needed before I can set res.

I'm out of ideas so if anyone knows how to fix it, I'd love to know :)

  • Issue: Better uop coverage in the JIT optimizer #131798

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
tomasr8 added 2 commits April 11, 2025 22:18
@tomasr8
Split CALL_TYPE_1 into several uops.
67ae618
@tomasr8
Add news entry
c8ca787
@tomasr8
tomasr8 requested review from Fidget-Spinner and markshannon as code owners April 11, 2025 20:24
@bedevere-app bedevere-app Bot added the awaiting review label Apr 11, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Apr 11, 2025
Better uop coverage in the JIT optimizer #131798
Open
tomasr8
tomasr8 commented Apr 11, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Outdated
sym_set_null(null);
}

op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {

@tomasr8 tomasr8 Apr 11, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure why I cannot do this here:

Suggested change
op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {
op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused -- callable, unused, unused)) {

I get SyntaxError: Duplicate name unused, though bytecodes.c allows it 🤔

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Apr 11, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. Maybe unused isn't a meaningful value here like it is in bytecodes.c (maybe @markshannon knows)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Apr 14, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave it like this. I'll create an issue for Mark to update the cases generator and mention this line.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@tomasr8 tomasr8 Apr 14, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tomasr8 tomasr8 Apr 19, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appears to be fixed now thanks to #132615, I updated the PR

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
brandtbucher
brandtbucher requested changes Apr 11, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer_bytecodes.c Outdated
sym_set_null(null);
}

op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {

@brandtbucher brandtbucher Apr 11, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird. Maybe unused isn't a meaningful value here like it is in bytecodes.c (maybe @markshannon knows)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot removed the awaiting review label Apr 11, 2025
@bedevere-app

bedevere-app Bot commented Apr 11, 2025

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added the awaiting changes label Apr 11, 2025
tomasr8 and others added 3 commits April 11, 2025 22:44
@tomasr8 @brandtbucher
Simplify _GUARD_CALLABLE_TYPE_1
9e1bb98
Co-authored-by: Brandt Bucher <[email protected]>
@tomasr8
Improve _CALL_TYPE_1
8e03efc
@tomasr8
Fix test
8984add
brandtbucher
brandtbucher reviewed Apr 12, 2025
View reviewed changes
Comment thread Lib/test/test_capi/test_opt.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher reviewed Apr 12, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 and others added 2 commits April 12, 2025 22:54
@tomasr8 @brandtbucher
PEP7
8a0152f
Co-authored-by: Brandt Bucher <[email protected]>
@tomasr8 @brandtbucher
Improve test
0f6c12b
Co-authored-by: Brandt Bucher <[email protected]>
tomasr8
tomasr8 commented Apr 12, 2025
View reviewed changes
Comment thread Python/optimizer_bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@tomasr8
Regenerate files
8097826
@tomasr8

tomasr8 commented Apr 14, 2025

Copy link
Copy Markdown
Member Author

Just to update the status:

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels Apr 14, 2025
@bedevere-app

bedevere-app Bot commented Apr 14, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app
bedevere-app Bot requested a review from brandtbucher April 14, 2025 08:44
brandtbucher
brandtbucher approved these changes Apr 14, 2025
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Up to you if you want to add the additional test. I'll leave this open for a bit longer just in case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels Apr 14, 2025
@tomasr8
Add a test
218dc00
@tomasr8

tomasr8 commented Apr 14, 2025

Copy link
Copy Markdown
Member Author

I added the test you suggested since it was quite easy to do :)

Also, should we care about the compiler warnings I mentioned in the PR description? There are now two unused variable warnings:

In file included from Python/ceval.c:1121:
Python/executor_cases.c.h: In function ‘_PyEval_EvalFrameDefault’:
Python/executor_cases.c.h:5142:25: warning: variable ‘callable’ set but not used [-Wunused-but-set-variable]
 5142 |             _PyStackRef callable;
      |                         ^~~~~~~~
Python/executor_cases.c.h:5141:25: warning: variable ‘null’ set but not used [-Wunused-but-set-variable]
 5141 |             _PyStackRef null;
      |                         ^~~~
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented Apr 14, 2025

Copy link
Copy Markdown
Member

Yeah, we should fix those. Can you rename them to unused, and remove the two DEAD calls, and add an INPUTS_DEAD() call at the very end, after the arg is closed?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tomasr8

tomasr8 commented Apr 14, 2025

Copy link
Copy Markdown
Member Author

Yeah, we should fix those. Can you rename them to unused, and remove the two DEAD calls, and add an INPUTS_DEAD() call at the very end, after the arg is closed?

it's getting a bit late here so maybe I missed something, but I'm getting SyntaxError: Input 'null' is still live when output 'res' is defined. FTR, this is what I did:

diff --git a/Python/bytecodes.c b/Python/bytecodes.c
index 22bb4097611..129e606bde2 100644
--- a/Python/bytecodes.c
+++ b/Python/bytecodes.c
@@ -3969,15 +3969,14 @@ dummy_func(
             DEOPT_IF(callable_o != (PyObject *)&PyType_Type);
         }
 
-        op(_CALL_TYPE_1, (callable, null, arg -- res)) {
+        op(_CALL_TYPE_1, (unused, unused, arg -- res)) {
             PyObject *arg_o = PyStackRef_AsPyObjectBorrow(arg);
 
             assert(oparg == 1);
-            DEAD(null);
-            DEAD(callable);
             STAT_INC(CALL, hit);
             res = PyStackRef_FromPyObjectNew(Py_TYPE(arg_o));
             PyStackRef_CLOSE(arg);
+            INPUTS_DEAD();
         }
 
         macro(CALL_TYPE_1) =
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tomasr8

tomasr8 commented Apr 15, 2025

Copy link
Copy Markdown
Member Author

How about we add

(void)null;
(void)callable;

I've seen it used in some other instructions. Not sure whether it's the best fix but it does get rid of the warnings.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 added 3 commits April 19, 2025 22:09
@tomasr8
Merge branch 'main' into jit-call-type-1
3bbfcf5
@tomasr8
unused2 -> unused
77e9439
@tomasr8
Silence compiler warnings
f9bd678
brandtbucher
brandtbucher approved these changes Apr 22, 2025
View reviewed changes

@brandtbucher brandtbucher left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just one more suggestion (then you should merge in main and make regen-cases to fix the conflicts).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/optimizer_bytecodes.c Outdated
Comment on lines +849 to +851
op(_CALL_TYPE_1, (callable, null, arg -- res)) {
(void)callable;
(void)null;

@brandtbucher brandtbucher Apr 22, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work!

Suggested change
op(_CALL_TYPE_1, (callable, null, arg -- res)) {
(void)callable;
(void)null;
op(_CALL_TYPE_1, (unused, unused, arg -- res)) {

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tomasr8 tomasr8 Apr 22, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that doesn't help silence the warnings in executor_cases.c.h :/ only thing that I found that works is (void)foo like in 0925179. Is there a better way?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher Apr 22, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, that's fine!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tomasr8 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
tomasr8 added 4 commits April 22, 2025 17:06
@tomasr8
Mark args as unused
e863c2c
@tomasr8
Merge remote-tracking branch 'upstream/main' into jit-call-type-1
5860e1b
@tomasr8
Regen uop ids
162dfec
@tomasr8
More compiler warnings..
0925179
brandtbucher
brandtbucher approved these changes Apr 22, 2025
View reviewed changes
@brandtbucher
brandtbucher merged commit a6a3dbb into python:main Apr 22, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Apr 22, 2025
@tomasr8
tomasr8 deleted the jit-call-type-1 branch April 22, 2025 17:54
This was referenced Apr 23, 2025
gh-131798: JIT: Split CALL_STR_1 into several uops #132849
Merged
gh-131798: JIT: Split CALL_TUPLE_1 into several uops #132851
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher approved these changes
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner Fidget-Spinner is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@tomasr8 @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.