gh-143732: add specialization for FOR_ITER#148745
gh-143732: add specialization for FOR_ITER#148745NekoAsakura wants to merge 9 commits intopython:mainfrom
FOR_ITER#148745Conversation
NekoAsakura
requested review from
Fidget-Spinner,
markshannon,
savannahostrowski and
tomasr8
as code owners
cocolato
left a comment
cocolato
left a comment
There was a problem hiding this comment.
LGTM, thanks for doing this!
| } | ||
|
|
||
| macro(FOR_ITER) = _SPECIALIZE_FOR_ITER + _FOR_ITER; | ||
| macro(FOR_ITER) = _SPECIALIZE_FOR_ITER + _RECORD_NOS_GEN_FUNC + _RECORD_NOS_TYPE + _FOR_ITER; |
There was a problem hiding this comment.
cpython/Python/optimizer_symbols.c
Lines 725 to 726 in ad7d361
We don't need
_RECORD_NOS_GEN_FUNC here, because the GEN_FUNC_TAG recorded here does not contribute to the current optimization.
There was a problem hiding this comment.
Every FOR_ITER specialisation's record list must be a prefix of FOR_ITER's.
_RECORD_NOS_GEN_FUNC writes a gen func or NULL to slot 0, matching what FOR_ITER_GEN reads from it.
https://github.com/python/cpython/actions/runs/24623062259/job/71997168333
| PyType_Watch(TYPE_WATCHER_ID, (PyObject *)probable); | ||
| _Py_BloomFilter_Add(dependencies, probable); | ||
| sym_set_type(iter, probable); | ||
| int32_t orig_target = (this_instr - 1)->target; |
There was a problem hiding this comment.
We should add an assert to make sure the last uop is _RECORD_NOS_TYPE
markshannon
left a comment
markshannon
left a comment
There was a problem hiding this comment.
I've a few suggestions inline.
| } | ||
|
|
||
| tier2 op(_ITER_NEXT_INLINE, (iternext_fn/4, iter, null_or_index -- iter, null_or_index, next)) { | ||
| volatile iternextfunc iternext_v = (iternextfunc)iternext_fn; |
There was a problem hiding this comment.
Why volatile? It shouldn't be necessary.
Also function pointers may not be the same size as normal pointers.
Can you add assert(sizeof(iternextfunc) == sizeof(uintptr_t)); to be on the safe side.
There was a problem hiding this comment.
Because mmap pages usually sit too far from tp_iternext for the offset to reach, we have to use volatile to force compiler to emit callq *%rax (target read from a register) instead of callq <rel32> (target baked in as a fixed offset). Otherwise the call jumps to the wrong address and segfaults.
|
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 And if you don't make the requested changes, you will be poked with soft cushions! |
Documentation build overview
92 files changed ·
|
3 participants
Uh oh!
There was an error while loading. Please reload this page.