◐ Shell
reader mode source ↗
Skip to content

GH-104584: Remove ip_offset and simplify EXIT_TRACE#108961

Closed
brandtbucher wants to merge 1 commit into
python:mainfrom
brandtbucher:ip-offset
Closed

GH-104584: Remove ip_offset and simplify EXIT_TRACE#108961
brandtbucher wants to merge 1 commit into
python:mainfrom
brandtbucher:ip-offset

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Sep 5, 2023

Copy link
Copy Markdown
Member

We don't need to keep ip_offset in a local, since it's known statically during trace construction. The only time that we don't know it is when pushing a frame for an unknown code object, so modify _PUSH_FRAME to set the prev_instr of the frame being pushed (this doesn't have any overhead, since it was always being followed by a SAVE_IP anyways). Otherwise, we just set the operand of SAVE_IP to the actual pointer that should be stored.

Also, EXIT_TRACE behaves the same as an unconditional goto deoptimize. Changing it to just do that instead makes things simpler, since all returns from tier two happen at labels (rather than tied up in the uop handlers).

@brandtbucher brandtbucher added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Sep 5, 2023
@brandtbucher brandtbucher self-assigned this Sep 5, 2023

@gvanrossum gvanrossum left a comment

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.

Hide comment

Sorry for the chatty review -- I have two angles, one about still being able to make sense of the debug output, another about whether _PUSH_FRAME is the right place to set prev_instr from the new code object. And some mumblings about wanting @markshannon to review this too (I'm not sure I 100% follow his reasoning for the LOAD/STORE macros he recently added to ceval_macros.h.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

awaiting core review interpreter-core (Objects, Python, Grammar, and Parser dirs) skip news

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants