gh-131798: JIT: Propagate the result in _BINARY_OP_SUBSCR_TUPLE_INT

[tomasr8]

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

foo = (1, 2)
x = foo[0]  # _BINARY_OP_SUBSCR_TUPLE_INT

we can now deduce that x is 1.

• Issue: Better uop coverage in the JIT optimizer #131798

[tomasr8]

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

[tomasr8]

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?

[Fidget-Spinner]

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.

[tomasr8]

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

[Fidget-Spinner]

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

[tomasr8]

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

[Fidget-Spinner]

Thanks!

[brandtbucher]

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

Suggested change

	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)

[tomasr8]

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?

[brandtbucher]

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

[brandtbucher]

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

[tomasr8]

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

[tomasr8]

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