Skip to content

bpo-39156: Break up COMPARE_OP into four logically distinct opcodes. #17754

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 4 commits into from
Jan 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 21 additions & 0 deletions Doc/library/dis.rst
Original file line number Diff line number Diff line change
Expand Up @@ -927,6 +927,20 @@ All of the following opcodes use their arguments.
``cmp_op[opname]``.


.. opcode:: IS_OP (invert)

Performs ``is`` comparison, or ``is not`` if ``invert`` is 1.

.. versionadded:: 3.9


.. opcode:: CONTAINS_OP (invert)

Performs ``in`` comparison, or ``not in`` if ``invert`` is 1.

.. versionadded:: 3.9


.. opcode:: IMPORT_NAME (namei)

Imports the module ``co_names[namei]``. TOS and TOS1 are popped and provide
Expand Down Expand Up @@ -961,6 +975,13 @@ All of the following opcodes use their arguments.

.. versionadded:: 3.1

.. opcode:: JUMP_IF_NOT_EXC_MATCH (target)

Tests whether the second value on the stack is an exception matching TOS,
and jumps if it is not. Pops two values from the stack.

.. versionadded:: 3.9


.. opcode:: JUMP_IF_TRUE_OR_POP (target)

Expand Down
8 changes: 3 additions & 5 deletions Include/opcode.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion Lib/importlib/_bootstrap_external.py
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ def _write_atomic(path, data, mode=0o666):
# Python 3.9a0 3420 (add LOAD_ASSERTION_ERROR #34880)
# Python 3.9a0 3421 (simplified bytecode for with blocks #32949)
# Python 3.9a0 3422 (remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY, POP_FINALLY bytecodes #33387)
# Python 3.9a2 3423 (add IS_OP, CONTAINS_OP and JUMP_IF_NOT_EXC_MATCH bytecodes #39156)
#
# MAGIC must change whenever the bytecode emitted by the compiler may no
# longer be understood by older implementations of the eval loop (usually
Expand All @@ -282,7 +283,7 @@ def _write_atomic(path, data, mode=0o666):
# Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
# in PC/launcher.c must also be updated.

MAGIC_NUMBER = (3422).to_bytes(2, 'little') + b'\r\n'
MAGIC_NUMBER = (3423).to_bytes(2, 'little') + b'\r\n'
_RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c

_PYCACHE = '__pycache__'
Expand Down
7 changes: 5 additions & 2 deletions Lib/opcode.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@
except ImportError:
pass

cmp_op = ('<', '<=', '==', '!=', '>', '>=', 'in', 'not in', 'is',
'is not', 'exception match', 'BAD')
cmp_op = ('<', '<=', '==', '!=', '>', '>=')

hasconst = []
hasname = []
Expand Down Expand Up @@ -159,6 +158,10 @@ def jabs_op(name, op):

name_op('LOAD_GLOBAL', 116) # Index in name list

def_op('IS_OP', 117)
def_op('CONTAINS_OP', 118)

jabs_op('JUMP_IF_NOT_EXC_MATCH', 121)
jrel_op('SETUP_FINALLY', 122) # Distance to target address

def_op('LOAD_FAST', 124) # Local variable number
Expand Down
141 changes: 70 additions & 71 deletions Lib/test/test_dis.py

Large diffs are not rendered by default.

12 changes: 6 additions & 6 deletions Lib/test/test_peepholer.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,14 +65,14 @@ def unot(x):
self.check_lnotab(unot)

def test_elim_inversion_of_is_or_in(self):
for line, cmp_op in (
('not a is b', 'is not',),
('not a in b', 'not in',),
('not a is not b', 'is',),
('not a not in b', 'in',),
for line, cmp_op, invert in (
('not a is b', 'IS_OP', 1,),
('not a is not b', 'IS_OP', 0,),
('not a in b', 'CONTAINS_OP', 1,),
('not a not in b', 'CONTAINS_OP', 0,),
):
code = compile(line, '', 'single')
self.assertInBytecode(code, 'COMPARE_OP', cmp_op)
self.assertInBytecode(code, cmp_op, invert)
self.check_lnotab(code)

def test_global_as_constant(self):
Expand Down
6 changes: 3 additions & 3 deletions Lib/test/test_positional_only_arg.py
Original file line number Diff line number Diff line change
Expand Up @@ -434,11 +434,11 @@ def g():
def f(x: not (int is int), /): ...

# without constant folding we end up with
# COMPARE_OP(is), UNARY_NOT
# with constant folding we should expect a COMPARE_OP(is not)
# COMPARE_OP(is), IS_OP (0)
# with constant folding we should expect a IS_OP (1)
codes = [(i.opname, i.argval) for i in dis.get_instructions(g)]
self.assertNotIn(('UNARY_NOT', None), codes)
self.assertIn(('COMPARE_OP', 'is not'), codes)
self.assertIn(('IS_OP', 1), codes)


if __name__ == "__main__":
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
Split the COMPARE_OP bytecode instruction into four distinct instructions.

* COMPARE_OP for rich comparisons
* IS_OP for 'is' and 'is not' tests
* CONTAINS_OP for 'in' and 'is not' tests
* JUMP_IF_NOT_EXC_MATCH for checking exceptions in 'try-except' statements.

This improves the clarity of the interpreter and should provide a modest
speedup.
3 changes: 2 additions & 1 deletion PC/launcher.c
Original file line number Diff line number Diff line change
Expand Up @@ -1247,6 +1247,7 @@ static PYC_MAGIC magic_values[] = {
{ 3360, 3379, L"3.6" },
{ 3390, 3399, L"3.7" },
{ 3400, 3419, L"3.8" },
{ 3420, 3429, L"3.9" },
{ 0 }
};

Expand Down Expand Up @@ -1830,7 +1831,7 @@ process(int argc, wchar_t ** argv)

#if !defined(VENV_REDIRECT)
/* bpo-35811: The __PYVENV_LAUNCHER__ variable is used to
* override sys.executable and locate the original prefix path.
* override sys.executable and locate the original prefix path.
* However, if it is silently inherited by a non-venv Python
* process, that process will believe it is running in the venv
* still. This is the only place where *we* can clear it (that is,
Expand Down
137 changes: 78 additions & 59 deletions Python/ceval.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ static void maybe_dtrace_line(PyFrameObject *, int *, int *, int *);
static void dtrace_function_entry(PyFrameObject *);
static void dtrace_function_return(PyFrameObject *);

static PyObject * cmp_outcome(PyThreadState *, int, PyObject *, PyObject *);
static PyObject * import_name(PyThreadState *, PyFrameObject *,
PyObject *, PyObject *, PyObject *);
static PyObject * import_from(PyThreadState *, PyObject *, PyObject *);
Expand Down Expand Up @@ -2896,19 +2895,95 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
}

case TARGET(COMPARE_OP): {
assert(oparg <= Py_GE);
PyObject *right = POP();
PyObject *left = TOP();
PyObject *res = cmp_outcome(tstate, oparg, left, right);
PyObject *res = PyObject_RichCompare(left, right, oparg);
SET_TOP(res);
Py_DECREF(left);
Py_DECREF(right);
SET_TOP(res);
if (res == NULL)
goto error;
PREDICT(POP_JUMP_IF_FALSE);
PREDICT(POP_JUMP_IF_TRUE);
DISPATCH();
}

case TARGET(IS_OP): {
PyObject *right = POP();
PyObject *left = TOP();
int res = (left == right)^oparg;
PyObject *b = res ? Py_True : Py_False;
Py_INCREF(b);
SET_TOP(b);
Py_DECREF(left);
Py_DECREF(right);
PREDICT(POP_JUMP_IF_FALSE);
PREDICT(POP_JUMP_IF_TRUE);
FAST_DISPATCH();
}

case TARGET(CONTAINS_OP): {
PyObject *right = POP();
PyObject *left = POP();
int res = PySequence_Contains(right, left);
Py_DECREF(left);
Py_DECREF(right);
if (res < 0) {
goto error;
}
PyObject *b = (res^oparg) ? Py_True : Py_False;
Py_INCREF(b);
PUSH(b);
PREDICT(POP_JUMP_IF_FALSE);
PREDICT(POP_JUMP_IF_TRUE);
FAST_DISPATCH();
}

#define CANNOT_CATCH_MSG "catching classes that do not inherit from "\
"BaseException is not allowed"

case TARGET(JUMP_IF_NOT_EXC_MATCH): {
PyObject *right = POP();
PyObject *left = POP();
if (PyTuple_Check(right)) {
Py_ssize_t i, length;
length = PyTuple_GET_SIZE(right);
for (i = 0; i < length; i++) {
PyObject *exc = PyTuple_GET_ITEM(right, i);
if (!PyExceptionClass_Check(exc)) {
_PyErr_SetString(tstate, PyExc_TypeError,
CANNOT_CATCH_MSG);
Py_DECREF(left);
Py_DECREF(right);
goto error;
}
}
}
else {
if (!PyExceptionClass_Check(right)) {
_PyErr_SetString(tstate, PyExc_TypeError,
CANNOT_CATCH_MSG);
Py_DECREF(left);
Py_DECREF(right);
goto error;
}
}
int res = PyErr_GivenExceptionMatches(left, right);
Py_DECREF(left);
Py_DECREF(right);
if (res > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this check for res > 0 should not be included.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why?

/* Exception matches -- Do nothing */;
}
else if (res == 0) {
JUMPTO(oparg);
}
else {
goto error;
}
DISPATCH();
}

case TARGET(IMPORT_NAME): {
PyObject *name = GETITEM(names, oparg);
PyObject *fromlist = POP();
Expand Down Expand Up @@ -4952,62 +5027,6 @@ _PyEval_SliceIndexNotNone(PyObject *v, Py_ssize_t *pi)
return 1;
}


#define CANNOT_CATCH_MSG "catching classes that do not inherit from "\
"BaseException is not allowed"

static PyObject *
cmp_outcome(PyThreadState *tstate, int op, PyObject *v, PyObject *w)
{
int res = 0;
switch (op) {
case PyCmp_IS:
res = (v == w);
break;
case PyCmp_IS_NOT:
res = (v != w);
break;
case PyCmp_IN:
res = PySequence_Contains(w, v);
if (res < 0)
return NULL;
break;
case PyCmp_NOT_IN:
res = PySequence_Contains(w, v);
if (res < 0)
return NULL;
res = !res;
break;
case PyCmp_EXC_MATCH:
if (PyTuple_Check(w)) {
Py_ssize_t i, length;
length = PyTuple_Size(w);
for (i = 0; i < length; i += 1) {
PyObject *exc = PyTuple_GET_ITEM(w, i);
if (!PyExceptionClass_Check(exc)) {
_PyErr_SetString(tstate, PyExc_TypeError,
CANNOT_CATCH_MSG);
return NULL;
}
}
}
else {
if (!PyExceptionClass_Check(w)) {
_PyErr_SetString(tstate, PyExc_TypeError,
CANNOT_CATCH_MSG);
return NULL;
}
}
res = PyErr_GivenExceptionMatches(v, w);
break;
default:
return PyObject_RichCompare(v, w, op);
}
v = res ? Py_True : Py_False;
Py_INCREF(v);
return v;
}

static PyObject *
import_name(PyThreadState *tstate, PyFrameObject *f,
PyObject *name, PyObject *fromlist, PyObject *level)
Expand Down
Loading