gh-100239: replace BINARY_SUBSCR & family by BINARY_OP with oparg NB_SUBSCR #129379
Conversation
iritkatriel
commented
Jan 27, 2025
•
iritkatriel
commented
- Issue: Specialize long tail of binary operations using a table. #100239
iritkatriel
requested review from
ericsnowcurrently,
Fidget-Spinner and
markshannon
as code owners
| return 0; | ||
| } | ||
| PyListObject* list = (PyListObject*)container; | ||
| Py_ssize_t index = index_obj->long_value.ob_digit[0]; |
There was a problem hiding this comment.
Do we need to check the index in the guard? We need to check it is nonnegative, but the other checks are handled also in the action by _PyList_GetItemRef. There will be no deopt, but that is no issue perhaps.
There was a problem hiding this comment.
We use the guard both to decide whether to optimise with this action, and then we call it from the action to decide whether to deopt. So once we pass the guard we should know that we're not deopting.
| STAT_INC(BINARY_SUBSCR, hit); | ||
| Py_ssize_t index = ((PyLongObject*)sub)->long_value.ob_digit[0]; | ||
| PyListObject* list = (PyListObject*)container; | ||
| #ifdef Py_GIL_DISABLED |
There was a problem hiding this comment.
We can use _PyList_GetItemRef for both ft and normal builds.
There was a problem hiding this comment.
Seems right. @corona10 I copied this over from here https://github.com/python/cpython/blame/8e57877e3f43e80762196f6869526d0c1585783a/Python/bytecodes.c#L869
Do you remember why we have the two versions?
iritkatriel
force-pushed
the
binary_subscr
branch
from
ed60aa8 to
a5b12bd
Compare
| PyListObject* list = (PyListObject*)container; | ||
| #ifdef Py_GIL_DISABLED | ||
| PyObject *res_o = _PyList_GetItemRef(list, index); | ||
| assert(res_o != NULL); |
There was a problem hiding this comment.
What if the list is resized after the guard has been executed and before the action is executed. Then _PyList_GetItemRef can return NULL.
|
Closing in favour of #129700. |
