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 4 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
| 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
| 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(...) | |
| ... |
| type == &PyTuple_Type) | ||
| { | ||
| if (type == &PyList_Type) { | ||
| ADD_OP(_BINARY_SLICE_LIST, 0, 0); |
There was a problem hiding this comment.
After factoring out, you can write
ADD_OP(_UNPACK_INDICES,...)
ADD_OP(_BINARY_SLICE_LIST,...)
...
Misc/NEWS.d/next/Core_and_Builtins/2026-02-08-13-14-00.gh-issue-144569.pjlJVe.rst
Outdated
Show resolved
Hide resolved
…e-144569.pjlJVe.rst
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.