Type propagation: just because something is const doesn't mean it automatically matches the type

[Fidget-Spinner]

Bug report

Bug description:

See log files for failure:

https://github.com/python/cpython/actions/runs/8021737661/job/21914497578

An example failure from my Windows machine:

Assertion failed: PyFloat_CheckExact(sym_get_const(left)), file C:\Users\Ken\Documents\GitHub\cpython\Python\tier2_redundancy_eliminator_cases.c.h, line 279
Fatal Python error: Aborted

Just because a constant is present, doesn't mean it's the right type. In such a case, I think we should bail from the abstract interpreter, because it's a guaranteed deopt.

E.g.

    op(_GUARD_BOTH_FLOAT, (left, right -- left, right)) {
        if (sym_matches_type(left, &PyFloat_Type) &&
            sym_matches_type(right, &PyFloat_Type)) {
            REPLACE_OP(this_instr, _NOP, 0 ,0);
        }
        sym_set_type(left, &PyFloat_Type);
        sym_set_type(right, &PyFloat_Type);
    }

should become

    op(_GUARD_BOTH_FLOAT, (left, right -- left, right)) {
        if (sym_matches_type(left, &PyFloat_Type) &&
            sym_matches_type(right, &PyFloat_Type)) {
            REPLACE_OP(this_instr, _NOP, 0 ,0);
        }
        if (sym_is_const(left)) {
            if (!sym_const_is_type(left, &PyFloat_Type) goto guaranteed_deopt;
        }
        if (sym_is_const(right)) {
            if (!sym_const_is_type(right, &PyFloat_Type) goto guaranteed_deopt;
        }
        sym_set_type(left, &PyFloat_Type);
        sym_set_type(right, &PyFloat_Type);
    }

While

    op(_BINARY_OP_ADD_FLOAT, (left, right -- res)) {
        if (sym_is_const(left) && sym_is_const(right)) {
            assert(PyFloat_CheckExact(sym_get_const(left)));
            assert(PyFloat_CheckExact(sym_get_const(right)));
            PyObject *temp = PyFloat_FromDouble(
                PyFloat_AS_DOUBLE(sym_get_const(left)) +
                PyFloat_AS_DOUBLE(sym_get_const(right)));
            ERROR_IF(temp == NULL, error);
            OUT_OF_SPACE_IF_NULL(res = sym_new_const(ctx, temp));
            // TODO gh-115506:
            // replace opcode with constant propagated one and update tests!
        }
        else {
            OUT_OF_SPACE_IF_NULL(res = sym_new_known_type(ctx, &PyFloat_Type));
        }
    }

should become

    op(_BINARY_OP_ADD_FLOAT, (left, right -- res)) {
        if (sym_is_const(left) && sym_is_const(right)) {
            if(!PyFloat_CheckExact(sym_get_const(left))) {
                goto guaranteed_deopt;
            }
            if(!PyFloat_CheckExact(sym_get_const(right))) {
                goto guaranteed_deopt;
            }
            PyObject *temp = PyFloat_FromDouble(
                PyFloat_AS_DOUBLE(sym_get_const(left)) +
                PyFloat_AS_DOUBLE(sym_get_const(right)));
            ERROR_IF(temp == NULL, error);
            OUT_OF_SPACE_IF_NULL(res = sym_new_const(ctx, temp));
            // TODO gh-115506:
            // replace opcode with constant propagated one and update tests!
        }
        else {
            OUT_OF_SPACE_IF_NULL(res = sym_new_known_type(ctx, &PyFloat_Type));
        }
    }

CPython versions tested on:

CPython main branch

Operating systems tested on:

No response

Linked PRs

• gh-115859: Disable the tier 2 redundancy eliminator by default #115860
• gh-115859: Re-enable T2 optimizer pass by default #116062
• gh-115859: Fix test_type_inconsistency() when run multiple times #116079

[gvanrossum]

Heh, I just ran into this myself.

[gvanrossum]

As to the approach, the change to _GUARD_BOTH_FLOAT seems optional, but removing the asserts from _BINARY_OP_ADD_FLOAT is essential. (You could also use sym_matches_type() followed by sym_is_const() there.)

I'd say go ahead and fix it.

[markshannon]

Please, please, let's stop hacking at the optimizer until we have proper documentation and tests for the symbols and lattice.
We are just going around in cycles "fixing" one problem by introducing another.

The code for _GUARD_BOTH_FLOAT is correct:
If both left and right are known to be floats, then replacing the uop with _NOP is correct. After _GUARD_BOTH_FLOAT both left are right will be known to be floats.

[Fidget-Spinner]

It put up a PR to turn off the tier 2 redundancy eliminator by default #115860.

[markshannon]

The existing code for _BINARY_OP_ADD_INT might be correct as well (apart from the leak)
In order to reach _BINARY_OP_ADD_INT both left and right must be ints.
Assuming that sys_is_const(top) == false, then sys_is_const(left) implies that left is an int constant and the code is correct.

[gvanrossum]

Reopening — if you enable the optimizer it still has this bug.

[gvanrossum]

I wonder what explains this crash:

test_create_server_multiple_hosts_ipv4 (test.test_asyncio.test_events.ProactorEventLoopTests.test_create_server_multiple_hosts_ipv4) ... Assertion failed: PyFloat_CheckExact(sym_get_const(left)), file D:\a\cpython\cpython\Python\tier2_redundancy_eliminator_cases.c.h, line 279

(It's in case _BINARY_OP_MULTIPLY_FLOAT.)

That sure looks like the redundancy eliminator encountered a situation where sys_is_const(left) is true (and sys_is_const(right) too), but the symbol left represents a constant of a different type than float.

I'm guessing we've projected a trace containing _BINARY_OP_ADD_FLOAT but some preceding uops in the trace lead the symbolic evaluator to infer that the stack entry corresponding to left is some other constant. I don't understand how that happened, since the Tier 1 specializer clearly though it was a float, but I can't prove this is impossible. Which is why I believe those _BINARY_OP_* opcodes in the redundancy eliminator should not contain those assert() calls.

That said, I am totally in favor of setting up more rigid testing for the primitives first before we continue tinkering.

[brandtbucher]

Could it be a situation like this?

X = 0.0
def f():
    for _ in range(16):
        X + X
# Specialize for float:
f()
# Make X an int:
X = 0
# Kick it into tier two.
# We have a float specialization but an int "constant" created from the global load:
f()

Probably also possible to do with something like f.__code__ = f.__code__.replace(co_consts=...).

[gvanrossum]

Exactly -- specialize T1 for float but invoke T2 with int.

I don't think that updating __code__ would do this -- the replace() will use the unspecialized T1 bytecode with all counters reset.