GeezerVison

I'm not sure these asserts are actually needed. The instruction will deopt when this is not true so perhaps we can remove them?

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?

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.

Got it, thanks! I somehow didn't realize you can know something is a tuple but not know its length..

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.

#132733

I haven't fully digested your PR yet, but maybe there is some way to extend it to support tuples as well?

@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):

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?

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. :)

Ah, I completely misunderstood, sorry. I thought you were checking whether the tuple was const, not the index. Disregard!

no worries :) I think we can still simplify the code a bit though!

@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?