Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Mar 14, 2025
Merged

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

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
222c9f5
move LOAD_SMALL_INT optimization from codegen to cfg
WolframAlph Feb 17, 2025
f878d78
fix conflicts
WolframAlph Mar 13, 2025
e3f4f8b
address review
WolframAlph Mar 14, 2025
fa005f3
check PyLong_AsLongAndOverflow result for error
WolframAlph Mar 14, 2025
83ccea7
decref newconst on error
WolframAlph Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions Python/flowgraph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,21 +1387,28 @@ nop_out(cfg_instr **instrs, int size)
}
}

/* Does not steal reference to "newconst" */
static bool
/* Does not steal reference to "newconst".
Return 1 if changed instruction to LOAD_SMALL_INT.
Return 0 if could not change instruction to LOAD_SMALL_INT.
Return -1 on error.
*/
static int
maybe_instr_make_load_smallint(cfg_instr *instr, PyObject *newconst,
PyObject *consts, PyObject *const_cache)
{
if (PyLong_CheckExact(newconst)) {
int overflow;
long val = PyLong_AsLongAndOverflow(newconst, &overflow);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (val == -1 && PyErr_Occurred()) {
return -1;
}
if (!overflow && _PY_IS_SMALL_INT(val)) {
assert(_Py_IsImmortal(newconst));
INSTR_SET_OP1(instr, LOAD_SMALL_INT, (int)val);
return true;
return 1;
}
}
return false;
return 0;
}


Expand All @@ -1410,9 +1417,9 @@ static int
instr_make_load_const(cfg_instr *instr, PyObject *newconst,
PyObject *consts, PyObject *const_cache)
{
if (maybe_instr_make_load_smallint(instr, newconst, consts, const_cache)) {
assert(instr->i_opcode == LOAD_SMALL_INT);
return SUCCESS;
int res = maybe_instr_make_load_smallint(instr, newconst, consts, const_cache);
if (res) {
return res == -1 ? ERROR : SUCCESS;
}
int oparg = add_const(newconst, consts, const_cache);
RETURN_IF_ERROR(oparg);
Expand Down Expand Up @@ -2054,8 +2061,11 @@ basicblock_optimize_load_const(PyObject *const_cache, basicblock *bb, PyObject *
cfg_instr *inst = &bb->b_instr[i];
if (inst->i_opcode == LOAD_CONST) {
PyObject *constant = get_const_value(inst->i_opcode, inst->i_oparg, consts);
(void)maybe_instr_make_load_smallint(inst, constant, consts, const_cache);
int res = maybe_instr_make_load_smallint(inst, constant, consts, const_cache);
Py_DECREF(constant);
Copy link
Contributor Author

@WolframAlph WolframAlph Feb 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (res < 0) {
return ERROR;
}
Comment on lines +2070 to +2072
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RETURN_IF_ERROR(res);

Copy link
Contributor Author

@WolframAlph WolframAlph Mar 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok.

}
bool is_copy_of_load_const = (opcode == LOAD_CONST &&
inst->i_opcode == COPY &&
Expand Down
Loading