-
-
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
Changes from 1 commit
f59751d
383c1fc
c5db6bc
683d8f7
a271ebc
977e639
bd19d79
658ea17
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1075,6 +1075,10 @@ static int | |
stack_effect(int opcode, int oparg, int jump) | ||
{ | ||
if (0 <= opcode && opcode < 256) { | ||
if (_PyOpcode_Deopt[opcode] != opcode) { | ||
// Specialized instructions are not supported. | ||
return PY_INVALID_STACK_EFFECT; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be correct to return the stack effect of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: # All not defined opcodes
for code in set(range(256)) - set(dis.opmap.values()):
with self.subTest(opcode=code):
self.assertRaises(ValueError, stack_effect, code)
self.assertRaises(ValueError, stack_effect, code, 0) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Ah yes. Wow, we need to fix that. |
||
} | ||
int popped, pushed; | ||
if (jump > 0) { | ||
popped = _PyOpcode_num_popped(opcode, oparg, true); | ||
|
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.
We have these:
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.
Okay, I'll use
opcode <= MAX_REAL_OPCODE
. I feel we don't needIS_PSEUDO_OPCODE()
because the switch will take care of that.