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.