Skip to content

gh-139393: fix _CALL_LEN JIT tests for tuples #139394

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
picnixz merged 3 commits into from
Sep 28, 2025
Merged

gh-139393: fix _CALL_LEN JIT tests for tuples #139394

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3162b3e
fix `_CALL_LEN` JIT tests for tuples
picnixz Sep 28, 2025
ea1f0e9
remove unused export
picnixz Sep 28, 2025
be5e820
Update Lib/test/test_capi/test_opt.py
picnixz Sep 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions Lib/test/test_capi/test_opt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@

_testinternalcapi = import_helper.import_module("_testinternalcapi")

from _testinternalcapi import TIER2_THRESHOLD
from _testinternalcapi import _PY_NSMALLPOSINTS, TIER2_THRESHOLD

#For test of issue 136154
GLOBAL_136154 = 42
Expand Down Expand Up @@ -2093,6 +2093,10 @@ def testfunc(n):
self.assertNotIn("_GUARD_TOS_INT", uops)

def test_call_len_known_length_small_int(self):
# Make sure that len(t) is optimized for a tuple of length 5.
# See https://github.com/python/cpython/issues/139393.
self.assertGreater(_PY_NSMALLPOSINTS, 5)

def testfunc(n):
x = 0
for _ in range(n):
Expand All @@ -2113,13 +2117,17 @@ def testfunc(n):
self.assertNotIn("_POP_TOP_LOAD_CONST_INLINE_BORROW", uops)

def test_call_len_known_length(self):
# Make sure that len(t) is optimized for a tuple of length 2048.
Comment thread
picnixz marked this conversation as resolved.
Outdated
# See https://github.com/python/cpython/issues/139393.
self.assertLess(_PY_NSMALLPOSINTS, 2048)

def testfunc(n):
class C:
t = tuple(range(300))
t = tuple(range(2048))

@ZeroIntensity ZeroIntensity Sep 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of using a hardcoded magic number, it would be nice if we used _PY_NSMALLPOSINTS * 2 or something like that to make sure this test always works, even if we increase _PY_NSMALLPOSINTS in the future.

@picnixz picnixz Sep 28, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I am actually unsure whether we need hardcoding here actually because of the JIT. I'll check if changing it to a compile-time known value is necessary or not.

@picnixz picnixz Sep 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah that's what I feared. While the test still passes, the uops will be different. I don't remember when we have some inlined computations x * y that are then transformed in a single LOAD_CONST.

@picnixz picnixz Sep 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Also, I think we should make such change at other places as well. If you want to, let's open a separate issue.


x = 0
for _ in range(n):
if len(C.t) == 300: # comparison + guard removed
if len(C.t) == 2048: # comparison + guard removed
x += 1
return x

Expand Down
9 changes: 9 additions & 0 deletions Modules/_testinternalcapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include "pycore_pyerrors.h" // _PyErr_ChainExceptions1()
#include "pycore_pylifecycle.h" // _PyInterpreterConfig_InitFromDict()
#include "pycore_pystate.h" // _PyThreadState_GET()
#include "pycore_runtime_structs.h" // _PY_NSMALLPOSINTS, _PY_NSMALLNEGINTS
#include "pycore_unicodeobject.h" // _PyUnicode_TransformDecimalAndSpaceToASCII()

#include "clinic/_testinternalcapi.c.h"
Expand Down Expand Up @@ -2576,6 +2577,14 @@ module_exec(PyObject *module)
return 1;
}

if (PyModule_AddIntMacro(module, _PY_NSMALLPOSINTS) < 0) {
return 1;
}

if (PyModule_AddIntMacro(module, _PY_NSMALLNEGINTS) < 0) {
return 1;
}

@ZeroIntensity ZeroIntensity Sep 28, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't used anywhere, is it?

@picnixz picnixz Sep 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Mmh, yes, this one isn't used anymore. I wondered whether I needed one but I don't need to. I'll amend this one.

@picnixz picnixz Sep 28, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Side comment: this function should return -1 instead of 1 as it's the case for other Py_mod_exec functions (I got surprised by the "return 1"). It doesn't really matter per se, but I think we should generally document what kind of values we expect for Py_mod_* handlers and tp_* ones (it's not clear what tp_clear should return and when).

Currently, errors in Py_mod_exec functions are indicated by a non-zero return value, but it's not documented. I would advise that we document this.


return 0;
}

Expand Down
Loading