Skip to content
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

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

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.

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