Repository navigation
gh-131798: JIT: Propagate the result in _BINARY_OP_SUBSCR_TUPLE_INT - #133003
Fidget-Spinner merged 4 commits into
Conversation
Sorry, something went wrong.
| assert(index >= 0); | ||
| assert(index < sym_tuple_length(left)); |
There was a problem hiding this comment.
I'm not sure these asserts are actually needed. The instruction will deopt when this is not true so perhaps we can remove them?
Sorry, something went wrong.
There was a problem hiding this comment.
Interesting, we are actually hitting the assertion:
Python/optimizer_cases.c.h:630: optimize_uops: Assertion `index < sym_tuple_length(left)' failed.I guess I need to move the DEOPT_IF checks into a separate guard for it to work?
Sorry, something went wrong.
There was a problem hiding this comment.
Check out the code for sym_tuple_length. It can return -1 if the length is not known. So for example if you propagate a PyTuple_Type, but not the length, it will fail.
You need to check that the length is not -1.
Sorry, something went wrong.
There was a problem hiding this comment.
Got it, thanks! I somehow didn't realize you can know something is a tuple but not know its length..
Sorry, something went wrong.
| long index = PyLong_AsLong(sym_get_const(ctx, right)); | ||
| assert(index >= 0); | ||
| assert(index < sym_tuple_length(left)); | ||
| res = sym_tuple_getitem(ctx, left, index); |
There was a problem hiding this comment.
This is a bit unfortunate. I would wish we could automatically evaluate this, but it seems tuples are a special category so we can't.
Sorry, something went wrong.
There was a problem hiding this comment.
I haven't fully digested your PR yet, but maybe there is some way to extend it to support tuples as well?
Sorry, something went wrong.
|
Thanks! |
Sorry, something went wrong.
| if (sym_is_const(ctx, sub_st)) { | ||
| assert(PyLong_CheckExact(sym_get_const(ctx, sub_st))); | ||
| long index = PyLong_AsLong(sym_get_const(ctx, sub_st)); | ||
| assert(index >= 0); | ||
| int tuple_length = sym_tuple_length(tuple_st); | ||
| if (tuple_length == -1) { | ||
| // Unknown length | ||
| res = sym_new_not_null(ctx); | ||
| } | ||
| else { | ||
| assert(index < tuple_length); | ||
| res = sym_tuple_getitem(ctx, tuple_st, index); | ||
| } | ||
| } | ||
| else { | ||
| res = sym_new_not_null(ctx); | ||
| } |
There was a problem hiding this comment.
@tomasr8, this can be improved to handle abstract tuples that may not be constant (and it cleans things up, since it includes a length check):
| if (sym_is_const(ctx, sub_st)) { | |
| assert(PyLong_CheckExact(sym_get_const(ctx, sub_st))); | |
| long index = PyLong_AsLong(sym_get_const(ctx, sub_st)); | |
| assert(index >= 0); | |
| int tuple_length = sym_tuple_length(tuple_st); | |
| if (tuple_length == -1) { | |
| // Unknown length | |
| res = sym_new_not_null(ctx); | |
| } | |
| else { | |
| assert(index < tuple_length); | |
| res = sym_tuple_getitem(ctx, tuple_st, index); | |
| } | |
| } | |
| else { | |
| res = sym_new_not_null(ctx); | |
| } | |
| long index = PyLong_AsLong(sym_get_const(ctx, | |
| sub_st)); | |
| // ...PyLong_AsLong can fail, need to check for error here... | |
| res = sym_tuple_getitem(ctx, tuple_st, index) |
Sorry, something went wrong.
There was a problem hiding this comment.
this can be improved to handle abstract tuples that may not be constant
@brandtbucher, I think this already handles abstract tuples? Assuming that abstract means those that have JIT_SYM_TUPLE_TAG rather than JIT_SYM_KNOWN_VALUE_TAG? For tuples that just have JIT_SYM_KNOWN_CLASS_TAG we don't know the length so there's nothing we can do I believe.
But yeah, we can simplify the code since _Py_uop_sym_tuple_getitem can handle unknown length. The only difference is that it sets the type to unknown rather than non null as we do here. Is that a problem? Shouldn't _Py_uop_sym_tuple_getitem always return at least JIT_SYM_NON_NULL_TAG?
Sorry, something went wrong.
There was a problem hiding this comment.
Your current code is gated on sym_is_const, which abstract tuples fail. The rest of your understanding is correct, though.
Is that a problem? Shouldn't _Py_uop_sym_tuple_getitem always return at least JIT_SYM_NON_NULL_TAG?
Sounds like you found your next PR. :)
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, I completely misunderstood, sorry. I thought you were checking whether the tuple was const, not the index. Disregard!
Sorry, something went wrong.
There was a problem hiding this comment.
no worries :) I think we can still simplify the code a bit though!
Sorry, something went wrong.
| if (tuple_length == -1) { | ||
| // Unknown length | ||
| res = sym_new_not_null(ctx); | ||
| } |
There was a problem hiding this comment.
@brandtbucher
Following the previous discussion, I'm wondering if it is correct to return sym_new_not_null here rather than sym_new_unknown?
We have an abstract tuple and we don't know its length so it is impossible to be sure that the index is not out of bounds for the tuple. Given that, I think it is incorrect to return sym_new_not_null here as regular Python code could raise an IndexError. Should we change this to sym_new_unknown?
Sorry, something went wrong.
More context: #132851 (comment)
This propagates the information about tuple elements after _BINARY_OP_SUBSCR_TUPLE_INT when the RHS is a constant.
For example in
we can now deduce that x is 1.