gh-131798: JIT: Narrow the return type of _CALL_LEN to int #132940
Conversation
Reduce unnecessary guards whenever `len()` is called and used in arithmetic operations.
diegorusso
requested review from
brandtbucher
and removed request for
Fidget-Spinner
|
Windows JIT builds might've been broken by #132852. That PR did not trigger JIT CI. |
| @@ -1055,6 +1055,10 @@ dummy_func(void) { | |||
| sym_set_const(callable, (PyObject *)&PyUnicode_Type); | |||
| } | |||
|
|
|||
| op(_CALL_LEN, (callable[1], self_or_null[1], args[oparg] -- res)) { | |||
There was a problem hiding this comment.
I think you can mark these as unused:
| op(_CALL_LEN, (callable[1], self_or_null[1], args[oparg] -- res)) { | |
| op(_CALL_LEN, (unused, unused, unused -- res)) { |
There was a problem hiding this comment.
Let's not do that. I prefer the inst definitions in this file to match bytecodes.c. In fact, it was always my intention to validate it, but if you check out the validation function in Tools/cases_generator/optimizer_generator.py, it says TODO 😉 . I just had no time to get around to do it.
There was a problem hiding this comment.
Ah, I didn't know that. I've been using unused in my recent PRs.. I'll stop doing that then :)
There was a problem hiding this comment.
yes, I tend to leave as closer as possible to the bytecodes.c
There was a problem hiding this comment.
Let's not do that. I prefer the inst definitions in this file to match
bytecodes.c. In fact, it was always my intention to validate it, but if you check out the validation function inTools/cases_generator/optimizer_generator.py, it says TODO 😉 . I just had no time to get around to do it.
Would this be mostly about ensuring the stack inputs and outputs are identical between bytecodes and optimizer_bytecodes? I can look into that if you want?
There was a problem hiding this comment.
Yes please. You'd need to add the code here https://github.com/python/cpython/blob/main/Tools/cases_generator/optimizer_generator.py#L33.
| a = [1, 2, 3, 4] | ||
| for _ in range(n): | ||
| _ = len(a) - 1 | ||
|
|
||
| _, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD) |
There was a problem hiding this comment.
I don't know if it's needed in this case, but for other tests there's normally a 'control' variable that checks that the result is still correct even with the optimization. Something like this:
| a = [1, 2, 3, 4] | |
| for _ in range(n): | |
| _ = len(a) - 1 | |
| _, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD) | |
| x = 0 | |
| a = [1, 2, 3, 4] | |
| for _ in range(n): | |
| _ = len(a) - 1 | |
| if _ == 3: | |
| x += 1 | |
| _, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD) | |
| self.assertEqual(res, TIER2_THRESHOLD) |
There was a problem hiding this comment.
Good point, I'll do it.
There was a problem hiding this comment.
OK the problem is if I add this, _GUARD_TOS_INT and _GUARD_NOS_INT are appearing in the uops list.
There was a problem hiding this comment.
Yeah we can't do it. x is from outside the loop (trace), so the first x += 1 has to have a guard somewhere.
Misc/NEWS.d/next/Core_and_Builtins/2025-04-25-14-56-45.gh-issue-131798.NpcKub.rst
Outdated
Show resolved
Hide resolved
…e-131798.NpcKub.rst Co-authored-by: Max Bernstein <[email protected]>
Reduce unnecessary guards whenever
len()is called and used in arithmetic operations.