gh-126835: Avoid creating unnecessary tuple when looking for constant sequence during constant folding

[WolframAlph]

• Issue: Move const folding to the peephole optimizer #126835

Context: #126835 (comment)

[WolframAlph]

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

[iritkatriel]

It's a good change. Some naming issues.

[iritkatriel]

It might make sense for instr_make_load_const to do the nop_out. It already changes the bytecode with INSTR_SET_OP1, so it makes sense for it to do the whole transformation.

It would need an arg for BINOP_OPERAND_COUNT, etc.

[WolframAlph]

I wouldn't do it. Functions do precisely what their names suggest. What if we would need to use instr_make_load_const in future without noping out?

[iritkatriel]

Then we can pass 0 for the "how many to NOP out" arg. We can also, if we want, add assertions in this function that the net change to stack effect of what we're doing is 0.

If you replace BUILD_TUPLE 5 by a LOAD_CONST without the NOPing, then you end up with invalid code.

[WolframAlph]

I see. My only concern is that function would be doing too much, and it's kind of hard to find a proper name for it to reflect everything it is doing.

[WolframAlph]

There is usage of nop_out without instr_make_load_const so I would probably leave this for another PR as it would require a bit of adjusting some unrelated (to this PR) code.

[iritkatriel]

My only concern is that function would be doing too much, and it's kind of hard to find a proper name for it to reflect everything it is doing.

The current name of the function doesn't reflect what it does (and that it can leave the bytecode in bad state).

There is usage of nop_out without instr_make_load_const so I would probably leave this for another PR as it would require a bit of adjusting some unrelated (to this PR) code.

You can just leave that usage as is. It won't be impacted by moving the nop_out in other places into instr_make_load_const.

Could also be in another PR.

[iritkatriel]

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

[WolframAlph]

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?

[WolframAlph]

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:

cpython/Python/flowgraph.c

Lines 386 to 405 in 98fa4a4

	#ifndef NDEBUG
	static bool
	cfg_builder_check(cfg_builder *g)
	{
	assert(g->g_entryblock->b_iused > 0);
	for (basicblock *block = g->g_block_list; block != NULL; block = block->b_list) {
	assert(!_PyMem_IsPtrFreed(block));
	if (block->b_instr != NULL) {
	assert(block->b_ialloc > 0);
	assert(block->b_iused >= 0);
	assert(block->b_ialloc >= block->b_iused);
	}
	else {
	assert (block->b_iused == 0);
	assert (block->b_ialloc == 0);
	}
	}
	return true;
	}
	#endif

So maybe just put it there?

[iritkatriel]

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

[WolframAlph]

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

[iritkatriel]

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

[iritkatriel]

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

[WolframAlph]

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.