GeezerVison

@iritkatriel what would be the good way to test this?

@iritkatriel what would be the good way to test this?

The existing functional tests should cover this because this PR changes only implementation details and not behaviour.

Might be worth making sure that we have tests for this on the boundaries of the relevant constants (_PY_STACK_USE_GUIDELINE and MIN_CONST_SEQUENCE_SIZE, so each of these and +-1).

MIN_CONST_SEQUENCE_SIZE is covered by test_optimize_if_const_set & test_optimize_if_const_list in test_peepholer.py.

_PY_STACK_USE_GUIDELINE is not covered anywhere AFAIK. I will add test in test_compiler_codegen.py for _PY_STACK_USE_GUIDELINE boundary.

Additionally, I would like to assert that we are not missing optimization by returning early if seq_size > _PY_STACK_USE_GUIDELINE. Basically small function counting how many consecutive constants there are and return count > _PY_STACK_USE_GUIDELINE. It would be stripped out on non debug builds however, so we would need to #ifdef it (or some other way (macro?)). WDYT?

It would be stripped out on non debug builds however, so we would need to #ifdef it

Actually, we already have such section in flowgraph.c:

So maybe just put it there?

Basically small function counting how many consecutive constants there are and return count > _PY_STACK_USE_GUIDELINE.

I think it's possible to have a long sequence of LOAD_CONSTS which are not later turned into a tuple, so this check is too harsh.

>>> def f():
...     g(1,2,3,4,5,6,7,8,9,10)
...     
>>> dis.dis(f)
  1           RESUME                   0

  2           LOAD_GLOBAL              1 (g + NULL)
              LOAD_SMALL_INT           1
              LOAD_SMALL_INT           2
              LOAD_SMALL_INT           3
              LOAD_SMALL_INT           4
              LOAD_SMALL_INT           5
              LOAD_SMALL_INT           6
              LOAD_SMALL_INT           7
              LOAD_SMALL_INT           8
              LOAD_SMALL_INT           9
              LOAD_SMALL_INT          10
              CALL                    10
              POP_TOP
              LOAD_CONST               0 (None)
              RETURN_VALUE

Then maybe assert constant sequence is within _PY_STACK_USE_GUIDELINE upon entering fold_tuple_of_constants & optimize_lists_and_sets?

static bool
const_folding_within_stack_use_guideline(basicblock *bb, int i)
{
    assert(i < bb->b_iused);

    int count = 0;
    for (; i >= 0; i--) {
        int opcode = bb->b_instr[i].i_opcode;
        if (opcode == NOP) {
            continue;
        }
        if (!loads_const(opcode)) {
            break;
        }
        count++;
    }
    
    return count <= _PY_STACK_USE_GUIDELINE;
}

static int
fold_tuple_of_constants(basicblock *bb, int i, PyObject *consts, PyObject *const_cache)
{
    /* Pre-conditions */
    assert(PyDict_CheckExact(const_cache));
    assert(PyList_CheckExact(consts));
    assert(const_folding_within_stack_use_guideline(bb, i-1));
    ...

I don't see what you're testing here that we're not covering with the normal tests.

I merged, so we won't get more conflicts. We can always add more tests later.

Alright. My point being that since we return early with if (seq_size > _PY_STACK_USE_GUIDELINE), maybe it's worth adding assertion to check that we are not missing optimization due to this. In case for some reason in future, codegen emits more consecutive constant stack loads then _PY_STACK_USE_GUIDELINE. But maybe I'm wrong and we don't need it.