gh-131798: JIT: Split `CALL_TYPE_1` into several uops #132419

tomasr8 · 2025-04-11T20:24:15Z

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

tomasr8 · 2025-04-11T20:25:31Z

Python/optimizer_bytecodes.c

+        sym_set_null(null);
+    }
+
+    op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {


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 🤔

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

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

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

Python/optimizer_bytecodes.c

brandtbucher · 2025-04-11T20:39:42Z

Python/optimizer_bytecodes.c

+        sym_set_null(null);
+    }
+
+    op(_GUARD_CALLABLE_TYPE_1, (callable, unused, unused2 -- callable, unused, unused2)) {


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

bedevere-app · 2025-04-11T20:39:51Z

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.

Co-authored-by: Brandt Bucher <[email protected]>

Lib/test/test_capi/test_opt.py

Python/optimizer_bytecodes.c

Co-authored-by: Brandt Bucher <[email protected]>

tomasr8 · 2025-04-12T21:03:39Z

Python/optimizer_bytecodes.c

+        if (sym_has_type(arg)) {
+            res = sym_new_const(ctx, (PyObject *)sym_get_type(arg));
+        }


Is there an easy way to test this part? i.e. that the res becomes const? AFAIK there are no other uops/guards that check for instances of type so we can't test it indirectly by checking for removed uops..

Yeah, none check for instances of type specifically... mostly because we haven't separated these out or added optimizer cases for them yet! ;)

I think we could still use the fact that it's a const to our advantage, though. Haven't tried this, but you might be able to write a test that does t = type(foo); if t is not None: .... I'm pretty sure the JIT will remove the _GUARD_IS_NOT_NONE_POP instruction when the value is a known constant like this.

tomasr8 · 2025-04-14T08:44:28Z

Just to update the status:

I have made the requested changes; please review again

bedevere-app · 2025-04-14T08:44:33Z

Thanks for making the requested changes!

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

brandtbucher

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

tomasr8 · 2025-04-14T19:29:30Z

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;
      |                         ^~~~

brandtbucher · 2025-04-14T20:52:26Z

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?

tomasr8 · 2025-04-14T21:22:33Z

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

tomasr8 · 2025-04-15T21:32:57Z

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.

tomasr8 added 2 commits April 11, 2025 22:18

Split CALL_TYPE_1 into several uops.

67ae618

Add news entry

c8ca787

tomasr8 requested review from Fidget-Spinner and markshannon as code owners April 11, 2025 20:24

bedevere-app bot added the awaiting review label Apr 11, 2025

bedevere-app bot mentioned this pull request Apr 11, 2025

Better uop coverage in the JIT optimizer #131798

Open

tomasr8 commented Apr 11, 2025

View reviewed changes

brandtbucher requested changes Apr 11, 2025

View reviewed changes

bedevere-app bot removed the awaiting review label Apr 11, 2025

bedevere-app bot added the awaiting changes label Apr 11, 2025

tomasr8 and others added 3 commits April 11, 2025 22:44

Simplify _GUARD_CALLABLE_TYPE_1

9e1bb98

Co-authored-by: Brandt Bucher <[email protected]>

Improve _CALL_TYPE_1

8e03efc

Fix test

8984add

brandtbucher reviewed Apr 12, 2025

View reviewed changes

Lib/test/test_capi/test_opt.py Outdated Show resolved Hide resolved

brandtbucher reviewed Apr 12, 2025

View reviewed changes

Python/optimizer_bytecodes.c Outdated Show resolved Hide resolved

tomasr8 and others added 2 commits April 12, 2025 22:54

PEP7

8a0152f

Co-authored-by: Brandt Bucher <[email protected]>

Improve test

0f6c12b

Co-authored-by: Brandt Bucher <[email protected]>

tomasr8 commented Apr 12, 2025

View reviewed changes

Regenerate files

8097826

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

bedevere-app bot requested a review from brandtbucher April 14, 2025 08:44

brandtbucher approved these changes Apr 14, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels Apr 14, 2025

Add a test

218dc00

tomasr8 added 3 commits April 19, 2025 22:09

Merge branch 'main' into jit-call-type-1

3bbfcf5

unused2 -> unused

77e9439

Silence compiler warnings

f9bd678

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-131798: JIT: Split `CALL_TYPE_1` into several uops #132419

gh-131798: JIT: Split `CALL_TYPE_1` into several uops #132419

tomasr8 commented Apr 11, 2025 •

edited by bedevere-app bot

Loading

tomasr8 Apr 11, 2025

brandtbucher Apr 11, 2025

brandtbucher Apr 14, 2025

tomasr8 Apr 14, 2025

tomasr8 Apr 19, 2025

brandtbucher Apr 11, 2025

bedevere-app bot commented Apr 11, 2025

tomasr8 Apr 12, 2025

brandtbucher Apr 14, 2025

tomasr8 commented Apr 14, 2025

bedevere-app bot commented Apr 14, 2025

brandtbucher left a comment

tomasr8 commented Apr 14, 2025

brandtbucher commented Apr 14, 2025

tomasr8 commented Apr 14, 2025

tomasr8 commented Apr 15, 2025

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

gh-131798: JIT: Split CALL_TYPE_1 into several uops #132419

Are you sure you want to change the base?

gh-131798: JIT: Split CALL_TYPE_1 into several uops #132419

Conversation

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

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

bedevere-app bot commented Apr 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasr8 commented Apr 14, 2025

bedevere-app bot commented Apr 14, 2025

brandtbucher left a comment

Choose a reason for hiding this comment

tomasr8 commented Apr 14, 2025

brandtbucher commented Apr 14, 2025

tomasr8 commented Apr 14, 2025

tomasr8 commented Apr 15, 2025

gh-131798: JIT: Split `CALL_TYPE_1` into several uops #132419

gh-131798: JIT: Split `CALL_TYPE_1` into several uops #132419

tomasr8 commented Apr 11, 2025 •

edited by bedevere-app bot

Loading