Skip to content

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.

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

Merged
merged 8 commits into from
Feb 9, 2023
Merged

gh-98831: Use opcode metadata for stack_effect() #101704

Show file tree
Hide file tree
Changes from all commits
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 Feb 8, 2023
383c1fc
Declare or define opcode metadata based on NEED_OPCODE_TABLES
gvanrossum Feb 8, 2023
c5db6bc
Use generated metadata for stack_effect()
gvanrossum Feb 8, 2023
683d8f7
compile.o depends on opcode_metadata.h
gvanrossum Feb 8, 2023
a271ebc
Return -1 for unknown opcode
gvanrossum Feb 8, 2023
977e639
Support jump<0 in stack_effect()
gvanrossum Feb 8, 2023
bd19d79
stack_effect() for specialized opcode is an error
gvanrossum Feb 8, 2023
658ea17
Use MAX_REAL_OPCODE as upper opcode limit
gvanrossum Feb 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 7 additions & 10 deletions Makefile.pre.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1445,24 +1445,21 @@ regen-opcode-targets:

.PHONY: regen-cases
regen-cases:
# Regenerate Python/generated_cases.c.h from Python/bytecodes.c
# Regenerate Python/generated_cases.c.h
# and Python/opcode_metadata.h
# from Python/bytecodes.c
# using Tools/cases_generator/generate_cases.py
PYTHONPATH=$(srcdir)/Tools/cases_generator \
$(PYTHON_FOR_REGEN) \
$(srcdir)/Tools/cases_generator/generate_cases.py \
-i $(srcdir)/Python/bytecodes.c \
-o $(srcdir)/Python/generated_cases.c.h.new
-o $(srcdir)/Python/generated_cases.c.h.new \
-m $(srcdir)/Python/opcode_metadata.h.new
$(UPDATE_FILE) $(srcdir)/Python/generated_cases.c.h $(srcdir)/Python/generated_cases.c.h.new
# Regenerate Python/opcode_metadata.h from Python/bytecodes.c
# using Tools/cases_generator/generate_cases.py --metadata
PYTHONPATH=$(srcdir)/Tools/cases_generator \
$(PYTHON_FOR_REGEN) \
$(srcdir)/Tools/cases_generator/generate_cases.py \
--metadata \
-i $(srcdir)/Python/bytecodes.c \
-o $(srcdir)/Python/opcode_metadata.h.new
$(UPDATE_FILE) $(srcdir)/Python/opcode_metadata.h $(srcdir)/Python/opcode_metadata.h.new

Python/compile.o: $(srcdir)/Python/opcode_metadata.h

Python/ceval.o: \
$(srcdir)/Python/ceval_macros.h \
$(srcdir)/Python/condvar.h \
Expand Down
258 changes: 38 additions & 220 deletions Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1074,135 +1074,49 @@ basicblock_next_instr(basicblock *b)
static int
stack_effect(int opcode, int oparg, int jump)
{
switch (opcode) {
case NOP:
case EXTENDED_ARG:
case RESUME:
case CACHE:
return 0;

/* Stack manipulation */
case POP_TOP:
return -1;
case SWAP:
return 0;
case END_FOR:
return -2;

/* Unary operators */
case UNARY_NEGATIVE:
case UNARY_NOT:
case UNARY_INVERT:
return 0;

case SET_ADD:
case LIST_APPEND:
return -1;
case MAP_ADD:
return -2;

case BINARY_SUBSCR:
return -1;
case BINARY_SLICE:
return -2;
case STORE_SUBSCR:
return -3;
case STORE_SLICE:
return -4;
case DELETE_SUBSCR:
return -2;

case GET_ITER:
return 0;

case LOAD_BUILD_CLASS:
return 1;
if (0 <= opcode && opcode <= MAX_REAL_OPCODE) {
if (_PyOpcode_Deopt[opcode] != opcode) {
// Specialized instructions are not supported.
return PY_INVALID_STACK_EFFECT;
Copy link
Member

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]?

Copy link
Member Author

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]?

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)

Copy link
Member

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.

Copy link
Member Author

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 or opmap, 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.

Copy link
Member

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.

Copy link
Member Author

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).

Copy link
Member

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.

}
int popped, pushed;
if (jump > 0) {
popped = _PyOpcode_num_popped(opcode, oparg, true);
pushed = _PyOpcode_num_pushed(opcode, oparg, true);
}
else {
popped = _PyOpcode_num_popped(opcode, oparg, false);
pushed = _PyOpcode_num_pushed(opcode, oparg, false);
}
if (popped < 0 || pushed < 0) {
return PY_INVALID_STACK_EFFECT;
}
if (jump >= 0) {
return pushed - popped;
}
if (jump < 0) {
// Compute max(pushed - popped, alt_pushed - alt_popped)
int alt_popped = _PyOpcode_num_popped(opcode, oparg, true);
int alt_pushed = _PyOpcode_num_pushed(opcode, oparg, true);
if (alt_popped < 0 || alt_pushed < 0) {
return PY_INVALID_STACK_EFFECT;
}
int diff = pushed - popped;
int alt_diff = alt_pushed - alt_popped;
if (alt_diff > diff) {
return alt_diff;
}
return diff;
}
}

case RETURN_VALUE:
return -1;
case RETURN_CONST:
return 0;
case SETUP_ANNOTATIONS:
return 0;
case YIELD_VALUE:
return 0;
// Pseudo ops
switch (opcode) {
case POP_BLOCK:
return 0;
case POP_EXCEPT:
return -1;

case STORE_NAME:
return -1;
case DELETE_NAME:
return 0;
case UNPACK_SEQUENCE:
return oparg-1;
case UNPACK_EX:
return (oparg&0xFF) + (oparg>>8);
case FOR_ITER:
return 1;
case SEND:
return jump > 0 ? -1 : 0;
case STORE_ATTR:
return -2;
case DELETE_ATTR:
return -1;
case STORE_GLOBAL:
return -1;
case DELETE_GLOBAL:
return 0;
case LOAD_CONST:
return 1;
case LOAD_NAME:
return 1;
case BUILD_TUPLE:
case BUILD_LIST:
case BUILD_SET:
case BUILD_STRING:
return 1-oparg;
case BUILD_MAP:
return 1 - 2*oparg;
case BUILD_CONST_KEY_MAP:
return -oparg;
case LOAD_ATTR:
return (oparg & 1);
case COMPARE_OP:
case IS_OP:
case CONTAINS_OP:
return -1;
case CHECK_EXC_MATCH:
return 0;
case CHECK_EG_MATCH:
return 0;
case IMPORT_NAME:
return -1;
case IMPORT_FROM:
return 1;

/* Jumps */
case JUMP_FORWARD:
case JUMP_BACKWARD:
case JUMP:
case JUMP_BACKWARD_NO_INTERRUPT:
case JUMP_NO_INTERRUPT:
return 0;

case JUMP_IF_TRUE_OR_POP:
case JUMP_IF_FALSE_OR_POP:
return jump ? 0 : -1;

case POP_JUMP_IF_NONE:
case POP_JUMP_IF_NOT_NONE:
case POP_JUMP_IF_FALSE:
case POP_JUMP_IF_TRUE:
return -1;

case COMPARE_AND_BRANCH:
return -2;

case LOAD_GLOBAL:
return (oparg & 1) + 1;

/* Exception handling pseudo-instructions */
case SETUP_FINALLY:
/* 0 in the normal flow.
Expand All @@ -1218,109 +1132,13 @@ stack_effect(int opcode, int oparg, int jump)
* of __(a)enter__ and push 2 values before jumping to the handler
* if an exception be raised. */
return jump ? 1 : 0;
case PREP_RERAISE_STAR:
return -1;
case RERAISE:
return -1;
case PUSH_EXC_INFO:
return 1;

case WITH_EXCEPT_START:
return 1;

case LOAD_FAST:
case LOAD_FAST_CHECK:
return 1;
case STORE_FAST:
return -1;
case DELETE_FAST:
return 0;

case RETURN_GENERATOR:
return 0;

case RAISE_VARARGS:
return -oparg;

/* Functions and calls */
case KW_NAMES:
return 0;
case CALL:
return -1-oparg;
case CALL_INTRINSIC_1:
return 0;
case CALL_FUNCTION_EX:
return -2 - ((oparg & 0x01) != 0);
case MAKE_FUNCTION:
return 0 - ((oparg & 0x01) != 0) - ((oparg & 0x02) != 0) -
((oparg & 0x04) != 0) - ((oparg & 0x08) != 0);
case BUILD_SLICE:
if (oparg == 3)
return -2;
else
return -1;

/* Closures */
case MAKE_CELL:
case COPY_FREE_VARS:
return 0;
case LOAD_CLOSURE:
return 1;
case LOAD_DEREF:
case LOAD_CLASSDEREF:
return 1;
case STORE_DEREF:
return -1;
case DELETE_DEREF:
return 0;

/* Iterators and generators */
case GET_AWAITABLE:
return 0;

case BEFORE_ASYNC_WITH:
case BEFORE_WITH:
return 1;
case GET_AITER:
return 0;
case GET_ANEXT:
return 1;
case GET_YIELD_FROM_ITER:
return 0;
case END_ASYNC_FOR:
return -2;
case CLEANUP_THROW:
return -2;
case FORMAT_VALUE:
/* If there's a fmt_spec on the stack, we go from 2->1,
else 1->1. */
return (oparg & FVS_MASK) == FVS_HAVE_SPEC ? -1 : 0;
case LOAD_METHOD:
return 1;
case LOAD_ASSERTION_ERROR:
return 1;
case LIST_EXTEND:
case SET_UPDATE:
case DICT_MERGE:
case DICT_UPDATE:
return -1;
case MATCH_CLASS:
return -2;
case GET_LEN:
case MATCH_MAPPING:
case MATCH_SEQUENCE:
case MATCH_KEYS:
return 1;
case COPY:
case PUSH_NULL:
return 1;
case BINARY_OP:
return -1;
case INTERPRETER_EXIT:
return -1;
default:
return PY_INVALID_STACK_EFFECT;
}

return PY_INVALID_STACK_EFFECT; /* not reachable */
}

Expand Down
25 changes: 18 additions & 7 deletions Python/opcode_metadata.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@
// from Python/bytecodes.c
// Do not edit!

#ifndef NDEBUG
static int
#ifndef NEED_OPCODE_TABLES
extern int _PyOpcode_num_popped(int opcode, int oparg, bool jump);
#else
int
_PyOpcode_num_popped(int opcode, int oparg, bool jump) {
switch(opcode) {
case NOP:
Expand Down Expand Up @@ -345,13 +347,15 @@ _PyOpcode_num_popped(int opcode, int oparg, bool jump) {
case CACHE:
return 0;
default:
Py_UNREACHABLE();
return -1;
}
}
#endif

#ifndef NDEBUG
static int
#ifndef NEED_OPCODE_TABLES
extern int _PyOpcode_num_pushed(int opcode, int oparg, bool jump);
#else
int
_PyOpcode_num_pushed(int opcode, int oparg, bool jump) {
switch(opcode) {
case NOP:
Expand Down Expand Up @@ -693,10 +697,11 @@ _PyOpcode_num_pushed(int opcode, int oparg, bool jump) {
case CACHE:
return 0;
default:
Py_UNREACHABLE();
return -1;
}
}
#endif

enum Direction { DIR_NONE, DIR_READ, DIR_WRITE };
enum InstructionFormat { INSTR_FMT_IB, INSTR_FMT_IBC, INSTR_FMT_IBC0, INSTR_FMT_IBC000, INSTR_FMT_IBC0000, INSTR_FMT_IBC00000000, INSTR_FMT_IBIB, INSTR_FMT_IX, INSTR_FMT_IXC, INSTR_FMT_IXC000 };
struct opcode_metadata {
Expand All @@ -705,7 +710,12 @@ struct opcode_metadata {
enum Direction dir_op3;
bool valid_entry;
enum InstructionFormat instr_format;
} _PyOpcode_opcode_metadata[256] = {
};

#ifndef NEED_OPCODE_TABLES
extern const struct opcode_metadata _PyOpcode_opcode_metadata[256];
#else
const struct opcode_metadata _PyOpcode_opcode_metadata[256] = {
[NOP] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IX },
[RESUME] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB },
[LOAD_CLOSURE] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB },
Expand Down Expand Up @@ -876,3 +886,4 @@ struct opcode_metadata {
[EXTENDED_ARG] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IB },
[CACHE] = { DIR_NONE, DIR_NONE, DIR_NONE, true, INSTR_FMT_IX },
};
#endif
Loading