GH-98831: Remove super-instruction definitions, use macro instructions instead#100124
Uh oh!
There was an error while loading. Please reload this page.
Conversation
brandtbucher commented Dec 8, 2022
Not gonna lie, the PR title scared me. |
gvanrossumforce-pushed the remove-supers branch from 6706827 to eadca51Comparegvanrossum changed the title 
gvanrossum commented Dec 9, 2022
Clearly I should use more provocative PR titles more often. :-) |
| #define_COMPARE_OP_FLOAT 1003 | ||
| #define_COMPARE_OP_INT 1004 | ||
| #define_COMPARE_OP_STR 1005 | ||
| #define_JUMP_IF 1006 |
There was a problem hiding this comment.
Should these just be zeroes, too? Or even -1, just to emphasize that they aren't real?
There was a problem hiding this comment.
Yeah, they should all be zeros. Since the value is never used I'm not sure what the point of -1 would be.
| TARGET(LOAD_FAST__LOAD_FAST){ | ||
| PyObject*_tmp_1; | ||
| PyObject*_tmp_2; | ||
| { |
There was a problem hiding this comment.
Random idea... any chance we could annotate the output of macros to make it easier to see what the different parts correspond to? So, like, this line would be:
| { | |
| {// LOAD_FAST |
And we would have {// JOIN and another {// LOAD_FAST below? Not sure how hard this is.
| // BEGIN BYTECODES // | ||
| op(JOIN, (word/1--)){ |
There was a problem hiding this comment.
Hmmm. It feels weird to treat the next instruction as a cache entry.
Why don't all macro components generate code to reload the incremented opcodes/opargs unconditionally? That way we wouldn't need JOIN at all. Or are we worried the compiler can't optimize it out when it's unused (or that garbage values could be too confusing)? Or does it mess up something else?
Perhaps there's a clean way of expressing in the DSL whether an op corresponds to a real instruction or not? Then we can just reload the opcode/oparg if that's true, and get rid of JOIN.
There was a problem hiding this comment.
Hmmm. It feels weird to treat the next instruction as a cache entry.
It's only weird if you think of the instruction stream as alternating instructions and cache entries. In the discussion about registers it was already proposed to use a "cache" entry to encode the operation (ADD, MUL, etc.) for BINARY_OP, so it's really just a stream of variable-length instructions. But yeah, this op is definitely full of internal hackery.
Why don't all macro components generate code to reload the incremented opcodes/opargs unconditionally? That way we wouldn't need JOIN at all. Or are we worried the compiler can't optimize it out when it's unused (or that garbage values could be too confusing)? Or does it mess up something else?
I worry more about where the next oparg would be loaded from. The code generator makes assumption about where next_instr points (for macros it keeps pointing just past the original instruction until the end, for supers it gets bumped for each component).
Perhaps there's a clean way of expressing in the DSL whether an op corresponds to a real instruction or not? Then we can just reload the opcode/oparg if that's true, and get rid of
JOIN.
That's not a bad idea -- because we actually have that way: inst vs op. The main downside would then be that there would no longer be a clue in the macro whether it defines a super or not.
| if (tkn:=self.expect(lx.IDENTIFIER)) andtkn.text=="super": | ||
| ifself.expect(lx.LPAREN): | ||
| iftkn:=self.expect(lx.IDENTIFIER): | ||
| ifself.expect(lx.RPAREN): | ||
| ifself.expect(lx.EQUALS): | ||
| ifops:=self.ops(): |
gvanrossum left a comment
gvanrossum left a comment There was a problem hiding this comment.
I'm feeling a lot of hesitation about this PR. Maybe we should just leave things as they were and focus on other work (e.g. finish converting more instructions to the explicit stack/cache effects form, and adding arrays).
| #define_COMPARE_OP_FLOAT 1003 | ||
| #define_COMPARE_OP_INT 1004 | ||
| #define_COMPARE_OP_STR 1005 | ||
| #define_JUMP_IF 1006 |
There was a problem hiding this comment.
Yeah, they should all be zeros. Since the value is never used I'm not sure what the point of -1 would be.
| // BEGIN BYTECODES // | ||
| op(JOIN, (word/1--)){ |
There was a problem hiding this comment.
Hmmm. It feels weird to treat the next instruction as a cache entry.
It's only weird if you think of the instruction stream as alternating instructions and cache entries. In the discussion about registers it was already proposed to use a "cache" entry to encode the operation (ADD, MUL, etc.) for BINARY_OP, so it's really just a stream of variable-length instructions. But yeah, this op is definitely full of internal hackery.
Why don't all macro components generate code to reload the incremented opcodes/opargs unconditionally? That way we wouldn't need JOIN at all. Or are we worried the compiler can't optimize it out when it's unused (or that garbage values could be too confusing)? Or does it mess up something else?
I worry more about where the next oparg would be loaded from. The code generator makes assumption about where next_instr points (for macros it keeps pointing just past the original instruction until the end, for supers it gets bumped for each component).
Perhaps there's a clean way of expressing in the DSL whether an op corresponds to a real instruction or not? Then we can just reload the opcode/oparg if that's true, and get rid of
JOIN.
That's not a bad idea -- because we actually have that way: inst vs op. The main downside would then be that there would no longer be a clue in the macro whether it defines a super or not.
gvanrossum commented Dec 12, 2022
Off-line we decided not to do this. Super-instructions may eventually disappear (when we have a register VM). |
3 participants
Replace all super-instructions with macros, using a special JOIN op to extract the next oparg. JOIN has a cache effect of one word that is equivalent to bumping next_instr.
Rip out all code for parsing and generating super-instructions.