10000 Changes to specialized instructions requires a magic number change. · Issue #109256 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

Changes to specialized instructions requires a magic number change. #109256

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.

Sign up for GitHub

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

Jump to bottom
Closed
markshannon opened this issue Sep 11, 2023 · 8 comments
Closed

Changes to specialized instructions requires a magic number change. #109256

markshannon opened this issue Sep 11, 2023 · 8 comments
Labels
3.13 bugs and security fixes

Comments

@markshannon
Copy link
Member
markshannon commented Sep 11, 2023 •
edited by bedevere-app bot
Loading

Until #107971 only changes to the set of instructions present in the pyc file required a new version change.
Now, removing or adding any opcode requires a pyc file.
There are a few problems with this.

  • pyc files have to be regenerated for changes unrelated to the on disk format.
  • This makes it much harder to add or remove specializations during the beta phase, as it will invalidate pyc files
  • It consumes an unnecessary number of magic numbers. This is not a big deal, but want to keep to a limit of 50 per release
  • It reduces stability in layout and thus performance, making meaningful benchmarking harder.
  • It is even easier to forget the magic number bump segfault on main for "./python.exe -m blurb" #109198

Proposal

Allocate "external" opcodes to odd numbers.
Allocate instrumented opcodes to the top region (as we do now)
Allocate specialized and other internal opcodes to even numbers

It would also be nice to be able to specify the most common opcodes, so that tooling does a decent job of laying out the tables.

We might also consider generating a hash of the opcodes and checking it test_opcodes to ensure that the magic number is updated when necessary.

@iritkatriel

Linked PRs
@markshannon markshannon added the 3.13 bugs and security fixes label Sep 11, 2023
@iritkatriel
Copy link
Member

What distinguishes external/internal opcodes in the DSL?

odd/even assumes at most half of each type. We could allocate ranges for them so it's more flexible.

@markshannon
Copy link
Member Author

What distinguishes external/internal opcodes in the DSL?

opcode._specializations has the information. It must have got it from somewhere

odd/even assumes at most half of each type. We could allocate ranges for them so it's more flexible.

Sure. It might be better as well as we have 117 normal instructions and 70 specializations, so it isn't a 50/50 split.

@iritkatriel
Copy link
Member

If we keep HAVE_ARGUMENT working, then we have the constraint that all ops with arg are below it and all ops without are above (except the instrumented). So it means we defined 4 ranges + one range for the instrumented instructions.

@iritkatriel
Copy link
Member

Unless we say that internal instructions don't respect this, and they go in the end before the instrumented ones.

@markshannon
Copy link
Member Author

Internal instructions don't exist as far as pyc files or dis are concerned, so I don't think HAVE_ARGUMENT is relevant.
They already went all over the place before #107971 as we just filled in the gaps with them.

Do we support HAVE_ARGUMENT any more?
Maybe we could define it as 0 and say that all instruction have an operand, it is just that some ignore their operand?

@markshannon
Copy link
Member Author
markshannon commented Sep 11, 2023 •
edited
Loading

If that's too radical, then deprecate HAVE_ARGUMENT, and we'll work around it for a couple more releases.

@iritkatriel
Copy link
Member

Internal instructions don't exist as far as pyc files or dis are concerned, so I don't think HAVE_ARGUMENT is relevant. They already went all over the place before #107971 as we just filled in the gaps with them.

Cool.

Do we support HAVE_ARGUMENT any more? Maybe we could define it as 0 and say that all instruction have an operand, it is just that some ignore their operand?

We do, for backward compatibility, but we also have dis.has_arg which is better.

@bedevere-app bedevere-app bot mentioned this issue Sep 11, 2023
Merged
iritkatriel added a commit that referenced this issue Sep 12, 2023
@iritkatriel
gh-109256: allocate opcode IDs for internal opcodes in their own range (
8b55adf 
#109269)
vstinner pushed a commit to vstinner/cpython that referenced this issue Sep 13, 2023
@iritkatriel @vstinner
pythongh-109256: allocate opcode IDs for internal opcodes in their ow…
547afcc 
…n range (python#109269)
@hugovk
Copy link
Member
hugovk commented Nov 9, 2023

Closing as the PR has been merged, please re-open if needed. Thanks!

@hugovk hugovk closed this as completed Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.13 bugs and security fixes
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@iritkatriel @hugovk @markshannon
0