Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

bpo-47120: define virtual opcodes in opcode.py. Better organization of jump opcodes. #32353

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

Closed
wants to merge 3 commits into from

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Apr 6, 2022

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

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

I'm not keen on this change.
It exposes a lot of highly changeable, internal details.

The "virtual" instructions don't exist outside of the compiler, so why expose them?
And why renumber all the jumps?

virtual = {}

RELATIVE = 0x1
ABSOLUTE = 0x2
Copy link
Member

Choose a reason for hiding this comment

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

Aren't we about to remove all absolute jumps?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, and then this flag will not be needed.

# (Pseudo-instructions used in the compiler,
# but turned into NOPs by the assembler)

def_op('SETUP_FINALLY', -1)
Copy link
Member

Choose a reason for hiding this comment

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

These are an internal detail of the compiler. They never exist in the final bytecode, so I don't think they should be here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The point was to autogenerate some of the macros around them.

Copy link
Member

Choose a reason for hiding this comment

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

If the code to generate the macro is large than macros, that suggests to me that it isn't worth it.

Copy link
Member Author

@iritkatriel iritkatriel Apr 6, 2022

Choose a reason for hiding this comment

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

For me it's not about how long the code is, but about how many places we need to change when we add/remove/change an opcode (i.e., maintainability).

@iritkatriel
Copy link
Member Author

And why renumber all the jumps?

We will need to do that if we want to use the evenness of a jump opcode to determine its direction (I'm not entirely sure we want to do that though).

@iritkatriel
Copy link
Member Author

I'll make the jumps change with the current scheme and we can refactor afterwards when the mess is more obvious.

@iritkatriel iritkatriel closed this Apr 6, 2022
@markshannon
Copy link
Member

Ultimately I expect that we will want all opcodes to be auto-generated from profiling information, to reduce the effective size of the dispatch table.

@iritkatriel iritkatriel deleted the jumps branch May 20, 2022 11:17
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.

4 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.