-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
gh-98831: Use opcode metadata for stack_effect() #101704
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
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
f59751d
Write output and metadata in a single run
gvanrossum 383c1fc
Declare or define opcode metadata based on NEED_OPCODE_TABLES
gvanrossum c5db6bc
Use generated metadata for stack_effect()
gvanrossum 683d8f7
compile.o depends on opcode_metadata.h
gvanrossum a271ebc
Return -1 for unknown opcode
gvanrossum 977e639
Support jump<0 in stack_effect()
gvanrossum bd19d79
stack_effect() for specialized opcode is an error
gvanrossum 658ea17
Use MAX_REAL_OPCODE as upper opcode limit
gvanrossum File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev
Previous commit
Use MAX_REAL_OPCODE as upper opcode limit
- Loading branch information
commit 658ea177d8213773e23926c3440d31e4daea70bf
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be correct to return the stack effect of
_PyOpcode_Deopt[opcode]
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so, but there are unit tests that insist that this is an error, and I didn't feel like changing the tests:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, those tests probably predate specialization. We don't need to change this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opcode.py goes to great lengths to hide the existence of specialized instructions -- they don't show in either
opname
oropmap
, even though pseudo ops do exist there. So I think it's by design. Though we could argue for changing that design.I found the algorithm in dis.py on L46-52. I think it's duplicated in Tools/build/generate_opcode_h.py on L145-151.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think dis.py:46-52 is actually allocating unused opcodes for the specialised instructions, while generate_opcode_h.py:145-151 is building the full deopt lookup table from opcode["_specializations"], plus mapping each normal opcode to itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh wait, in generate_opcode_h.py it's L101-107. These two algorithms ought to match, otherwise results will be hilarious. :-)
Anyway, I'm going to merge now. On to figuring out whether mark_stacks() is in reach yet (I doubt it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. Wow, we need to fix that.