gh-144569: Scalar replacement of BINARY_SLICE for list, tuple, and unicode#144590
gh-144569: Scalar replacement of BINARY_SLICE for list, tuple, and unicode#144590cocolato wants to merge 7 commits intopython:mainfrom
BINARY_SLICE for list, tuple, and unicode#144590Conversation
cocolato
requested review from
Fidget-Spinner,
markshannon,
savannahostrowski and
tomasr8
as code owners
|
I think the tests failure that occurred is unrelated to the PR. |
|
You forgot to add the new recorder to BINARY_SLICE macro op |
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
lgtm just one comment and add news please
Python/bytecodes.c
Outdated
| PySlice_AdjustIndices(PyUnicode_GET_LENGTH(container_o), &istart, &istop, 1); | ||
| res_o = PyUnicode_Substring(container_o, istart, istop); | ||
| } | ||
| DECREF_INPUTS(); |
There was a problem hiding this comment.
We should split this into the POP_TOP POP_TOP form, can you please do that in a follow up PR?
|
Actually I've changed my mind. Can you please open a PR to convert BINARY_SLICE to the POP_TOP form please? Then after we merge that PR we can update this one |
Fidget-Spinner
left a comment
Fidget-Spinner
left a comment
There was a problem hiding this comment.
Let's leave the DECREF_INPUTS alone for now and try to remove it in a future PR.
The code has repeated instances that should be factored out to a micro-op
Python/bytecodes.c
Outdated
| if (_PyEval_SliceIndex(PyStackRef_AsPyObjectBorrow(start), &istart) && | ||
| _PyEval_SliceIndex(PyStackRef_AsPyObjectBorrow(stop), &istop)) { | ||
| PySlice_AdjustIndices(PyList_GET_SIZE(container_o), &istart, &istop, 1); | ||
| res_o = PyList_GetSlice(container_o, istart, istop); | ||
| } |
There was a problem hiding this comment.
This is repeated code, it should be factored out to a _UNPACK_INDICES micro-op that looks like this:
| if (_PyEval_SliceIndex(PyStackRef_AsPyObjectBorrow(start), &istart) && | |
| _PyEval_SliceIndex(PyStackRef_AsPyObjectBorrow(stop), &istop)) { | |
| PySlice_AdjustIndices(PyList_GET_SIZE(container_o), &istart, &istop, 1); | |
| res_o = PyList_GetSlice(container_o, istart, istop); | |
| } | |
| int err = _PyEval_SliceIndex(PyStackRef_AsPyObjectBorrow(start), &istart); | |
| ERROR_IF(err == 0); | |
| err = _PyEval_SliceIndex(PyStackRef_AsPyObjectBorrow(stop), &istop); | |
| ERROR_IF(err = 0); | |
| ... | |
| istart = PyStackRef_Wrap(...) | |
| ... |
Misc/NEWS.d/next/Core_and_Builtins/2026-02-08-13-14-00.gh-issue-144569.pjlJVe.rst
Outdated
Show resolved
Hide resolved
|
This is pretty close, I'm going to push some changes in, then wait a day to see if Mark has any objections tomorrow. |
|
Thanks for your guidance! |
1. We cannot exit after unpacking indices, as the stack contains tagged ints now which may lead to a crash. We must insert a guard for the type before unpacking indices. 2. It is possible for an indice to not fit in a tagged int, in which case we must deopt. 3. Recorded values do not always mean that that's the actual type we will see at runtime. So we must guard on that as well.
|
@cocolato please check out the latest commit and the extended commit message for the bugs fixed. Thanks! |
|
Results using hyperfine: So we made it >10% faster. Nice! |
2 participants
When the tier 2 optimizer can determine the type of the container being sliced,
_BINARY_SLICEcan be replaced with a specialized micro-op that bypasses thePySliceObjectallocation entirely.This PR implements scalar replacement of
BINARY_SLICEby add new tier2 op_BINARY_SLICE_LIST,_BINARY_SLICE_TUPLE, and_BINARY_SLICE_LIST.