gh-130480: Move duplicate LOAD_SMALL_INT optimization from codegen to CFG

[WolframAlph]

• Issue: Move duplicate LOAD_SMALL_INT optimization from codegen to CFG #130480

cc @Eclips4@tomasr8

[WolframAlph]

For small ints this is effectively no-op as they are immortal, but it helps readability because get_const_value returns new reference.

[markshannon]

Overall, I think this change isn't worthwhile. It doesn't seem to simplify the code at all, but it will slow down the compiler by introducing extra values into the constant array, which then need removing again.

[WolframAlph]

@markshannon are we ok with having this logic in 2 places then? Also what is your take on #130016?

[markshannon]

What logic in two places?
Codegen emits unoptimized instructions, the cfg optimize optimizes them. I don't see any change to that in this PR.

[WolframAlph]

@markshannon this:

cpython/Python/flowgraph.c

Lines 1414 to 1422 in 8058390

	if (PyLong_CheckExact(newconst)) {
	intoverflow;
	longval=PyLong_AsLongAndOverflow(newconst, &overflow);
	if (!overflow&&_PY_IS_SMALL_INT(val)) {
	assert(_Py_IsImmortal(newconst));
	INSTR_SET_OP1(instr, LOAD_SMALL_INT, (int)val);
	returnSUCCESS;
	}
	}

&&

cpython/Python/codegen.c

Lines 284 to 291 in 8058390

	if (PyLong_CheckExact(o)) {
	intoverflow;
	longval=PyLong_AsLongAndOverflow(o, &overflow);
	if (!overflow&&_PY_IS_SMALL_INT(val)) {
	ADDOP_I(c, loc, LOAD_SMALL_INT, val);
	returnSUCCESS;
	}
	}

[WolframAlph]

@iritkatriel can I ask you to review?

[iritkatriel]

Why are the constants [1, None]? I thought we always have None in index 0 when there is no docstring.

[WolframAlph]

LOAD_CONST 0 (1) is added as the first constant, LOAD_CONST 1 (None) is added as the second constant by _PyCodegen_AddReturnAtEnd. Currently there is 1 slot that is always preserved for constant (in case it is docstring) at index 0. 1 is used constant, so it makes to the final co_consts list, and we have 2 elements in it. Since 1 came first (therefore has lower index), it is first in the final co_consts.

[WolframAlph]

There is #130016 that addresses unused docstring slot which causes confusion.

[iritkatriel]

The return value of PyLong_AsLongAndOverflow needs to be checked for error. Does this mean that maybe_instr_make_load_smallint should also return int, with -1 for error?

[WolframAlph]

The return value of PyLong_AsLongAndOverflow needs to be checked for error

I think it was not checked before due to PyLong_CheckExact guard, but I agree we should check it anyway. This is by the way not the only place where this pattern occurs, for example:

cpython/Modules/_cursesmodule.c

Lines 378 to 383 in 10cbd1f

	elseif (PyLong_CheckExact(obj)) {
	intlong_overflow;
	value=PyLong_AsLongAndOverflow(obj, &long_overflow);
	if (long_overflow)
	goto overflow;
	}

[WolframAlph]

Done.

[iritkatriel]

RETURN_IF_ERROR(res);

[WolframAlph]

maybe_instr_make_load_smallint does not return SUCCESS nor ERROR explicitly, so I am not sure we should do it like this.

[iritkatriel]

ok.