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 theDEOPT_IF checks into a separate guard for it to work?

Check out the code forsym_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 haveJIT_SYM_TUPLE_TAG rather thanJIT_SYM_KNOWN_VALUE_TAG? For tuples that just haveJIT_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 tounknown rather thannon null as we do here. Is that a problem? Shouldn't_Py_uop_sym_tuple_getitem always return at leastJIT_SYM_NON_NULL_TAG?

Your current code is gated onsym_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 leastJIT_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 theprevious discussion, I'm wondering if it is correct to returnsym_new_not_null here rather thansym_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 returnsym_new_not_null here as regular Python code could raise anIndexError. Should we change this tosym_new_unknown?