gh-131798: JIT: Split `CALL_ISINSTANCE` into several uops #133339

tomasr8 · 2025-05-03T11:54:14Z

Split CALL_ISINSTANCE into two guards and the uop itself.

It will be easier to implement #133172 with this in place.

Issue: Better uop coverage in the JIT optimizer #131798

brandtbucher

Thanks, I just see an opportunity for further cleanup!

brandtbucher · 2025-05-03T17:21:32Z

Python/bytecodes.c

+        op(_GUARD_CALLABLE_ISINSTANCE_NULL, (callable, null, unused[oparg] -- callable, null, unused[oparg])) {
+            DEOPT_IF(!PyStackRef_IsNull(null));
+        }


Since now we know that the oparg is two, we can split out the two args and get rid of the array. Also, let's give this a better name (since it's part of the same logical family as _GUARD_TOS_NULL and _GUARD_NOS_NULL).

Suggested change

op(_GUARD_CALLABLE_ISINSTANCE_NULL, (callable, null, unused[oparg] -- callable, null, unused[oparg])) {

DEOPT_IF(!PyStackRef_IsNull(null));

}

op(_GUARD_THIRD_NULL, (null, unused, unused -- null, unused, unused)) {

DEOPT_IF(!PyStackRef_IsNull(null));

}

Nice! I agree it makes sense to make it reusable. I also moved it just under _GUARD_NOS_NULL to help with discoverability

Python/bytecodes.c

brandtbucher · 2025-05-03T17:22:49Z

Python/bytecodes.c

            PyInterpreterState *interp = tstate->interp;
            DEOPT_IF(callable_o != interp->callable_cache.isinstance);
+        }
+
+        op(_CALL_ISINSTANCE, (callable, null, args[oparg] -- res)) {


And here:

Suggested change

op(_CALL_ISINSTANCE, (callable, null, args[oparg] -- res)) {

op(_CALL_ISINSTANCE, (callable, null, inst, cls -- res)) {

inst is a reserved keyword so I went with inst_ :)

Python/optimizer_bytecodes.c

bedevere-app · 2025-05-03T17:23:53Z

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.

tomasr8 · 2025-05-03T20:39:38Z

Python/bytecodes.c

            if (retval < 0) {
                ERROR_NO_POP();
            }
            (void)null; // Silence compiler warnings about unused variables
+            PyStackRef_CLOSE(cls);
+            PyStackRef_CLOSE(inst_);


Interesting that with named stackrefs it won't let me do this:

DEAD(null); PyStackRef_CLOSE(cls); PyStackRef_CLOSE(inst_);

(the error is SyntaxError: Input 'null' is not live, but 'inst_' is)
While with the previous args version this was fine:

DEAD(null); PyStackRef_CLOSE(args[0]); PyStackRef_CLOSE(args[1]);

I guess the cases generator can't reason about arrays? Another reason to use named stackrefs instead :)

tomasr8 · 2025-05-03T21:10:29Z

CI looks good so: I have made the requested changes; please review again :)

bedevere-app · 2025-05-03T21:10:34Z

Thanks for making the requested changes!

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

brandtbucher

One fix to the test:

Lib/test/test_capi/test_opt.py

brandtbucher · 2025-05-06T20:51:33Z

Python/bytecodes.c

            PyInterpreterState *interp = tstate->interp;
            DEOPT_IF(callable_o != interp->callable_cache.isinstance);
+        }
+
+        op(_CALL_ISINSTANCE, (callable, null, inst_, cls -- res)) {


I don't love the trailing underscore. Not a huge deal, but maybe just rename to instance or obj or something ?

I renamed it to instance in be50e24

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

Fidget-Spinner · 2025-05-08T17:21:42Z

I think the tests could be more robust if there was a way to run the executors without optimizing the uops. Each test could then first use assertIn to ensure the relevant uops are in fact present and only then run with optimizations enabled. Though dynamically turning off optimizations would require some changes in the interpreter so I'm not sure if it's worth it.

You could do this if you want. Mark wanted to do this, but no one has time to implement it proper.

The steps to do this would be the following:

Export some C API for optimize_uops in _textinternalcapi.c or something.
The API should take a bytecode offset/slice to start trace projection from.
Write our optimizer tests to just use that.

tomasr8 · 2025-05-08T17:49:39Z

Ok I'll give it a try! Seems like a nice project for the weekend (or for PyCon if I can't get it working by then 😄)

Thanks for writing down the individual steps for me, it's super helpful :)

tomasr8 · 2025-05-09T21:38:36Z

I think the tests could be more robust if there was a way to run the executors without optimizing the uops. Each test could then first use assertIn to ensure the relevant uops are in fact present and only then run with optimizations enabled. Though dynamically turning off optimizations would require some changes in the interpreter so I'm not sure if it's worth it.

You could do this if you want. Mark wanted to do this, but no one has time to implement it proper.

The steps to do this would be the following:
1. Export some C API for `optimize_uops` in `_textinternalcapi.c` or something.

2. The API should take a bytecode offset/slice to start trace projection from.

3. Write our optimizer tests to just use that.

@Fidget-Spinner, I noticed that the optimizer can already be turned off by setting PYTHON_UOPS_OPTIMIZE to '0':

cpython/Python/optimizer.c

Lines 1280 to 1288 in 98e2c3a

    
           char *env_var = Py_GETENV("PYTHON_UOPS_OPTIMIZE"); 
        
           if (env_var == NULL || *env_var == '\0' || *env_var > '0') { 
        
               length = _Py_uop_analyze_and_optimize(frame, buffer, 
        
                                                  length, 
        
                                                  curr_stackentries, &dependencies); 
        
               if (length <= 0) { 
        
                   return length; 
        
               } 
        
           }

Rather than exposing a new internal api, I think we could simply toggle this variable in the tests. I'm not sure if this is what Mark intended as this would still test more than just the optimizer proper. Though I like that these tests are more end-to-end.

Anyway, here's what it'd look like. Let me know if this is something we want to pursue, otherwise I'll go back to thinking how to expose just the optimizer :)

diff --git a/Lib/test/test_capi/test_opt.py b/Lib/test/test_capi/test_opt.py
index 651148336f7..b82b36fa2f5 100644
--- a/Lib/test/test_capi/test_opt.py
+++ b/Lib/test/test_capi/test_opt.py
@@ -11,6 +11,7 @@
 from test.support import (script_helper, requires_specialization,
                           import_helper, Py_GIL_DISABLED, requires_jit_enabled,
                           reset_code)
+from test.support.os_helper import EnvironmentVarGuard
 
 _testinternalcapi = import_helper.import_module("_testinternalcapi")
 
@@ -458,6 +459,12 @@ def _run_with_optimizer(self, testfunc, arg):
         ex = get_first_executor(testfunc)
         return res, ex
 
+    def _run_without_optimizer(self, testfunc, arg):
+        with EnvironmentVarGuard() as env:
+            env["PYTHON_UOPS_OPTIMIZE"] = "0"
+            res = testfunc(arg)
+        ex = get_first_executor(testfunc)
+        return res, ex
 
     def test_int_type_propagation(self):
         def testfunc(loops):
@@ -1951,6 +1958,17 @@ def testfunc(n):
                     x += 1
             return x
 
+        res, ex = self._run_without_optimizer(testfunc, TIER2_THRESHOLD)
+        self.assertEqual(res, TIER2_THRESHOLD)
+        self.assertIsNotNone(ex)
+        uops = get_opnames(ex)
+        self.assertIn("_CALL_ISINSTANCE", uops)
+        self.assertIn("_GUARD_THIRD_NULL", uops)
+        self.assertIn("_GUARD_CALLABLE_ISINSTANCE", uops)
+
+        # Invalidate the executor to force a reoptimization
+        _testinternalcapi.invalidate_executors(testfunc.__code__)
+
         res, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD)
         self.assertEqual(res, TIER2_THRESHOLD)
         self.assertIsNotNone(ex)

Fidget-Spinner · 2025-05-09T22:58:17Z

@tomasr8 yeah I'm not advocating for removing the end-to-end tests altogether. Rather, the optimizer tests should have a mix of both end-to-end and unit.

For example, it would be nice if we could generate an optimized trace without having to even wrap it in a for loop and run till JIT threshold. This actually makes the tests really slow because JIT threshold is quite high right now.

tomasr8 · 2025-05-10T10:00:11Z

Got it! Yeah, being able to simplify the tests and make them run faster is definitely worth it :)

tomasr8 added 3 commits May 3, 2025 13:34

Split CALL_ISIINSTANCE into several uops

43ec167

Add news entry

0ab70a9

Close all stackrefs

900472a

tomasr8 requested review from Fidget-Spinner and markshannon as code owners May 3, 2025 11:54

bedevere-app bot mentioned this pull request May 3, 2025

Better uop coverage in the JIT optimizer #131798

Open

bedevere-app bot added the awaiting review label May 3, 2025

tomasr8 mentioned this pull request May 3, 2025

gh-131798: JIT: Narrow the return type of isinstance for some known arguments #133172

Open

tomasr8 requested a review from brandtbucher May 3, 2025 17:18

brandtbucher requested changes May 3, 2025

View reviewed changes

bedevere-app bot added awaiting changes and removed awaiting review labels May 3, 2025

tomasr8 added 2 commits May 3, 2025 22:22

Rename to _GUARD_THIRD_NULL

6f49dca

Unpack args array into separate stack variables

8ba53c3

tomasr8 commented May 3, 2025

View reviewed changes

bedevere-app bot added awaiting change review and removed awaiting changes labels May 3, 2025

bedevere-app bot requested a review from brandtbucher May 3, 2025 21:10

brandtbucher reviewed May 6, 2025

View reviewed changes

brandtbucher changed the title ~~gh-131798: JIT: Split CALL_ISINSTANCE into severeal uops~~ gh-131798: JIT: Split CALL_ISINSTANCE into several uops May 6, 2025

tomasr8 and others added 4 commits May 8, 2025 18:54

Fix tests

6e11442

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

Rename parameter

be50e24

Merge remote-tracking branch 'upstream/main' into jit-split-isinstance

ec61bc5

Regen cases

b0b31dd

tomasr8 requested a review from brandtbucher May 8, 2025 20:18

brandtbucher approved these changes May 8, 2025

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting change review labels May 8, 2025

brandtbucher merged commit c492ac7 into python:main May 8, 2025
63 of 64 checks passed

bedevere-app bot removed the awaiting merge label May 8, 2025

tomasr8 deleted the jit-split-isinstance branch May 8, 2025 21:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-131798: JIT: Split `CALL_ISINSTANCE` into several uops #133339

gh-131798: JIT: Split `CALL_ISINSTANCE` into several uops #133339

tomasr8 commented May 3, 2025 •

edited by bedevere-app bot

Loading

brandtbucher left a comment

brandtbucher May 3, 2025 •

edited

Loading

tomasr8 May 3, 2025

brandtbucher May 3, 2025

tomasr8 May 3, 2025

bedevere-app bot commented May 3, 2025

tomasr8 May 3, 2025

tomasr8 commented May 3, 2025

bedevere-app bot commented May 3, 2025

brandtbucher left a comment

brandtbucher May 6, 2025

tomasr8 May 8, 2025

Fidget-Spinner commented May 8, 2025

tomasr8 commented May 8, 2025

tomasr8 commented May 9, 2025

Fidget-Spinner commented May 9, 2025

tomasr8 commented May 10, 2025

	op(_CALL_ISINSTANCE, (callable, null, args[oparg] -- res)) {
	op(_CALL_ISINSTANCE, (callable, null, inst, cls -- res)) {

gh-131798: JIT: Split CALL_ISINSTANCE into several uops #133339

gh-131798: JIT: Split CALL_ISINSTANCE into several uops #133339

Conversation

tomasr8 commented May 3, 2025 • edited by bedevere-app bot Loading

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher May 3, 2025 • edited Loading

Choose a reason for hiding this comment

tomasr8 May 3, 2025

Choose a reason for hiding this comment

brandtbucher May 3, 2025

Choose a reason for hiding this comment

tomasr8 May 3, 2025

Choose a reason for hiding this comment

bedevere-app bot commented May 3, 2025

tomasr8 May 3, 2025

Choose a reason for hiding this comment

tomasr8 commented May 3, 2025

bedevere-app bot commented May 3, 2025

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher May 6, 2025

Choose a reason for hiding this comment

tomasr8 May 8, 2025

Choose a reason for hiding this comment

Fidget-Spinner commented May 8, 2025

tomasr8 commented May 8, 2025

tomasr8 commented May 9, 2025

Fidget-Spinner commented May 9, 2025

tomasr8 commented May 10, 2025

gh-131798: JIT: Split `CALL_ISINSTANCE` into several uops #133339

gh-131798: JIT: Split `CALL_ISINSTANCE` into several uops #133339

tomasr8 commented May 3, 2025 •

edited by bedevere-app bot

Loading

brandtbucher May 3, 2025 •

edited

Loading