Skip to content

GH-115419: Move setting the instruction pointer to error exit stubs #118088

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.

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 3 commits into from
Apr 24, 2024

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Apr 19, 2024

Currently, in tier 2, we emit a _SET_IP before any instruction that may error so that the instruction pointer is correctly set.
Since errors are relatively rare, this is wasteful.

With this PR, the error handling uop _ERROR_POP_N sets the instruction pointer, so we don't need to emit the _SET_IP inline.

We still need to emit _SET_IP before instructions that escape.

@markshannon
Copy link
Member Author

markshannon commented Apr 19, 2024

Stats show a ~3% reduction in uops executed overall.

@markshannon
Copy link
Member Author

markshannon commented Apr 22, 2024

4 pre-existing failures and a timeout.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

The benchmark numbers don't mean anything, do they? IIUC only the stats matter here, and they look good. These benchmark numbers (3% slower) are for the Tier 2 interpreter. If you care to see if it made anything actually faster, run with JIT.

@markshannon
Copy link
Member Author

The benchmark numbers don't mean anything, do they?

No. We are only interested in tier 1 performance for 3.13. For 3.14 we are assuming that the JIT will be working well by then, so we are optimizing for tier uop stats.

@markshannon markshannon merged commit 83235f7 into python:main Apr 24, 2024
49 of 54 checks passed
@markshannon markshannon deleted the move-set-ip-to-exits branch August 6, 2024 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants