Skip to content

GH-132554: Fix tier2 FOR_ITER implementation and optimizations - #135137

Merged
markshannon merged 4 commits into
python:mainfrom
faster-cpython:fix-tier2-for-iter
Jun 5, 2025
Merged

GH-132554: Fix tier2 FOR_ITER implementation and optimizations#135137
markshannon merged 4 commits into
python:mainfrom
faster-cpython:fix-tier2-for-iter

Conversation

@markshannon

@markshannonmarkshannon commented Jun 4, 2025•
edited by bedevere-app Bot
Loading

Copy link
Copy Markdown
Member

When adding virtual iterators, the tier 1 and tier 2 implementations of FOR_ITER diverged. I've already fixed a problem where the instrumented FOR_ITER differed from the normal one.

To prevent these problems happening again, this PR factors out the majority of FOR_ITER into a helper function for the 3 versions of FOR_ITER to share.

I've also added PyStackRef_ERROR to distinguish between errors and no result and remove the need for an additional out parameter for the helper function.

Also fixes a bug in the code generator where there are three or more output values, one is an unchanged input, one is a changed input and one is undefined.

@bedevere-appbedevere-appBot mentioned this pull request Jun 4, 2025
Open
@markshannon
markshannon requested review from brandtbucher and removed request for Fidget-SpinnerJune 4, 2025 14:43
@Fidget-Spinner

Fidget-Spinner commented Jun 4, 2025

Copy link
Copy Markdown
Member

Can you check out the Windows JIT failures please? I'll review the PR after that.

brandtbucher
brandtbucher approved these changes Jun 5, 2025
View reviewed changes
Comment threadInclude/internal/pycore_ceval.h Outdated
#endif

_PyStackRef _PyForIter_NextWithIndex(PyObject *seq, _PyStackRef index);
_PyStackRef

@brandtbucherbrandtbucherJun 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
_PyStackRef
PyAPI_FUNC(_PyStackRef)

Comment threadPython/optimizer_bytecodes.c Outdated
op(_GET_ITER, (iterable -- iter, index_or_null)) {
if (sym_matches_type(iterable, &PyTuple_Type) || sym_matches_type(iterable, &PyList_Type)) {
iter = iterable;
index_or_null = sym_new_type(ctx, &PyLong_Type);

@brandtbucherbrandtbucherJun 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hm, this is sort of weird. We don't have a symbol for "unboxed int" in the JIT, but it really doesn't feel correct to type this as int. Maybe leave as unknown and we can update our lattice with unboxed C types later? It's not like this information is being used yet, anyways.

Suggested change
index_or_null=sym_new_type(ctx, &PyLong_Type);
index_or_null=sym_new_unknown(ctx);

Comment threadPython/bytecodes.c
ERROR_NO_POP();
}
next = PyStackRef_FromPyObjectSteal(next_o);
JUMPBY(oparg + 1);

@brandtbucherbrandtbucherJun 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you add back the comment that this is skipping the END_FOR?

Suggested change
JUMPBY(oparg+1);
// Jump forward by oparg, then skip the following END_FOR:
JUMPBY(oparg+1);

Comment threadPython/bytecodes.c
if (PyStackRef_IsError(item)) {
ERROR_NO_POP();
}
JUMPBY(oparg + 1);

@brandtbucherbrandtbucherJun 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
JUMPBY(oparg+1);
// Jump forward by oparg, then skip the following END_FOR:
JUMPBY(oparg+1);

Comment threadPython/ceval.c Outdated
_PyErr_Clear(tstate);
}
else {
return PyStackRef_ERROR;

@brandtbucherbrandtbucherJun 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
returnPyStackRef_ERROR;
returnPyStackRef_ERROR;

@brandtbucher

brandtbucher commented Jun 5, 2025

Copy link
Copy Markdown
Member

Confirmed that this fixed the pprint benchmarks locally. Just kicked off new benchmarks now.

@brandtbucher

brandtbucher commented Jun 5, 2025

Copy link
Copy Markdown
Member

27% slower... ;)

@markshannon
markshannon merged commit b90ecea into python:mainJun 5, 2025
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@markshannon@Pranjal095
pythonGH-132554: Fix tier2FOR_ITERimplementation and optimizations (
3557d29
pythonGH-135137)
taegyunkim pushed a commit to taegyunkim/cpython that referenced this pull request Aug 4, 2025
@markshannon@taegyunkim
pythonGH-132554: Fix tier2FOR_ITERimplementation and optimizations (
dd6f0ca
pythonGH-135137)
Agent-Hellboy pushed a commit to Agent-Hellboy/cpython that referenced this pull request Aug 19, 2025
@markshannon@Agent-Hellboy
pythonGH-132554: Fix tier2FOR_ITERimplementation and optimizations (
e7ac90f
pythonGH-135137)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucherbrandtbucherbrandtbucher approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@markshannon@Fidget-Spinner@brandtbucher