Skip to content

gh-104584: Support most jumping instructions #106393

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

Closed
wants to merge 9 commits into from
Closed

gh-104584: Support most jumping instructions #106393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d31d003
Hacky way to implement jump ops
gvanrossum Jul 2, 2023
b886ca6
Make JUMP_BACKWARD[_NO_INTERRUPT] viable
gvanrossum Jul 3, 2023
28b3951
Replace ENTER_EXECUTOR with the original in trace projection
gvanrossum Jul 3, 2023
0825696
Make POP_JUMP_IF_FALSE/TRUE conditionally jump
gvanrossum Jul 3, 2023
1df7b84
Implement JUMP_TO_TOP special uop
gvanrossum Jul 3, 2023
d6235ee
Cleanup some commented-out lines
gvanrossum Jul 4, 2023
6cd5719
Tweak FOR_ITER so it's viable
gvanrossum Jul 4, 2023
b33a1a2
Check room available for JUMP_TO_TOP
gvanrossum Jul 4, 2023
f5e18d8
A stray JUMP_BACKWARD also ends the trace
gvanrossum Jul 4, 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
47 changes: 21 additions & 26 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2219,17 +2219,17 @@ dummy_func(
}

inst(JUMP_FORWARD, (--)) {
JUMPBY(oparg);
JUMP_POP_DISPATCH(oparg, 0);
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave dispatch to the code generator?
The saved instruction pointer (frame->prev_instr) is part of the VM state like any other, so shouldn't need special casing. Unless the necessary information is not otherwise present. All the information necessary is in JUMPBY(oparg).

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, so you're saying that for Tier 2 the code generator should just replace JUMPBY(<expr>) with something Tier-2-appropriate. That could actually work. There are three places where we shouldn't do that, SEND, JUMP_BACKWARD and ENTER_EXECUTOR, we can exclude those by forbidding something they use.

Copy link
Member

Choose a reason for hiding this comment

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

SEND will probably need to be broken up into two micro ops, one that does the send, and another that does the jump. The SEND jump should be a more-or-less normal jump. We also need to track where we are sending from, to handle the matching YIELD_VALUE, so I'd "forbid" SEND for now.

I think you already handle JUMP_BACKWARD and ENTER_EXECUTOR correctly. They complete the loop, or exit.

}

inst(JUMP_BACKWARD, (--)) {
CHECK_EVAL_BREAKER();
#if ENABLE_SPECIALIZATION
_Py_CODEUNIT *here = next_instr - 1;
assert(oparg <= INSTR_OFFSET());
JUMPBY(1-oparg);
#if ENABLE_SPECIALIZATION
here[1].cache += (1 << OPTIMIZER_BITS_IN_COUNTER);
if (here[1].cache > tstate->interp->optimizer_backedge_threshold) {
JUMPBY(1 - oparg);
OBJECT_STAT_INC(optimization_attempts);
frame = _PyOptimizer_BackEdge(frame, here, next_instr, stack_pointer);
if (frame == NULL) {
Expand All @@ -2241,6 +2241,7 @@ dummy_func(
goto resume_frame;
}
#endif /* ENABLE_SPECIALIZATION */
JUMP_POP_DISPATCH(1 - oparg, 0);
}

pseudo(JUMP) = {
Expand Down Expand Up @@ -2272,24 +2273,28 @@ dummy_func(

inst(POP_JUMP_IF_FALSE, (cond -- )) {
assert(PyBool_Check(cond));
JUMPBY(oparg * Py_IsFalse(cond));
if (Py_IsFalse(cond)) {
JUMP_POP_DISPATCH(oparg, 1);
}
}

inst(POP_JUMP_IF_TRUE, (cond -- )) {
assert(PyBool_Check(cond));
JUMPBY(oparg * Py_IsTrue(cond));
if (Py_IsTrue(cond)) {
JUMP_POP_DISPATCH(oparg, 1);
}
}

inst(POP_JUMP_IF_NOT_NONE, (value -- )) {
if (!Py_IsNone(value)) {
DECREF_INPUTS();
JUMPBY(oparg);
JUMP_POP_DISPATCH(oparg, 1);
}
}

inst(POP_JUMP_IF_NONE, (value -- )) {
if (Py_IsNone(value)) {
JUMPBY(oparg);
JUMP_POP_DISPATCH(oparg, 1);
}
else {
DECREF_INPUTS();
Expand All @@ -2302,7 +2307,7 @@ dummy_func(
* generator or coroutine, so we deliberately do not check it here.
* (see bpo-30039).
*/
JUMPBY(-oparg);
JUMP_POP_DISPATCH(-oparg, 0);
}

inst(GET_LEN, (obj -- obj, len_o)) {
Expand Down Expand Up @@ -2410,18 +2415,17 @@ dummy_func(
if (!_PyErr_ExceptionMatches(tstate, PyExc_StopIteration)) {
goto error;
}
monitor_raise(tstate, frame, next_instr-1);
monitor_raise(tstate, frame, frame->prev_instr);
_PyErr_Clear(tstate);
}
/* iterator ended normally */
#if ENABLE_SPECIALIZATION
assert(next_instr[INLINE_CACHE_ENTRIES_FOR_ITER + oparg].op.code == END_FOR ||
next_instr[INLINE_CACHE_ENTRIES_FOR_ITER + oparg].op.code == INSTRUMENTED_END_FOR);
#endif
Py_DECREF(iter);
STACK_SHRINK(1);
SKIP_OVER(INLINE_CACHE_ENTRIES_FOR_ITER);
/* Jump forward oparg, then skip following END_FOR instruction */
JUMPBY(oparg + 1);
DISPATCH();
JUMP_POP_DISPATCH(INLINE_CACHE_ENTRIES_FOR_ITER + oparg + 1, 1);
}
// Common case: no jump, leave it to the code generator
}
Expand Down Expand Up @@ -2468,11 +2472,8 @@ dummy_func(
Py_DECREF(seq);
}
Py_DECREF(iter);
STACK_SHRINK(1);
SKIP_OVER(INLINE_CACHE_ENTRIES_FOR_ITER);
/* Jump forward oparg, then skip following END_FOR instruction */
JUMPBY(oparg + 1);
DISPATCH();
JUMP_POP_DISPATCH(INLINE_CACHE_ENTRIES_FOR_ITER + oparg + 1, 1);
end_for_iter_list:
// Common case: no jump, leave it to the code generator
}
Expand All @@ -2491,11 +2492,8 @@ dummy_func(
Py_DECREF(seq);
}
Py_DECREF(iter);
STACK_SHRINK(1);
SKIP_OVER(INLINE_CACHE_ENTRIES_FOR_ITER);
/* Jump forward oparg, then skip following END_FOR instruction */
JUMPBY(oparg + 1);
DISPATCH();
JUMP_POP_DISPATCH(INLINE_CACHE_ENTRIES_FOR_ITER + oparg + 1, 1);
end_for_iter_tuple:
// Common case: no jump, leave it to the code generator
}
Expand All @@ -2505,12 +2503,9 @@ dummy_func(
DEOPT_IF(Py_TYPE(r) != &PyRangeIter_Type, FOR_ITER);
STAT_INC(FOR_ITER, hit);
if (r->len <= 0) {
STACK_SHRINK(1);
// STACK_SHRINK(1);
Py_DECREF(r);
SKIP_OVER(INLINE_CACHE_ENTRIES_FOR_ITER);
// Jump over END_FOR instruction.
JUMPBY(oparg + 1);
DISPATCH();
JUMP_POP_DISPATCH(INLINE_CACHE_ENTRIES_FOR_ITER + oparg + 1, 1);
}
long value = r->start;
r->start = value + r->step;
Expand Down
17 changes: 17 additions & 0 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2715,6 +2715,10 @@ void Py_LeaveRecursiveCall(void)

///////////////////// Experimental UOp Interpreter /////////////////////

#undef JUMP_POP_DISPATCH
#define JUMP_POP_DISPATCH(x, n) \
Copy link
Member

Choose a reason for hiding this comment

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

This seems unnecessary.

The superblock generator should be converting each conditional jump into a conditional exit, followed by an unconditional jump.
Such that:

POP_JUMP_IF_TRUE label

becomes:

False branch more likely

EXIT_IF_TRUE

True branch more likely

EXIT_IF_FALSE
JUMP_FORWARD label

As for which branch is more likely, we will need to record which way branches go.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, just so I understand correctly, the idea is that EXIT_IF_TRUE/FALSE doesn't pop, and continues in the bytecode at the original POP_JUMP_IF_TRUE instruction, which will pop. So the "False branch more likely" (i.e., branch not likely) version will have to actually translate to EXIT_IF_TRUE; POP_TOP. We could special-case this in the translator: it would have put such a list of uops in the expansion for POP_JUMP_IF_TRUE and friends, and we'd have to add hand-written cases to the uop executor for EXIT_IF_TRUE etc.

In the other case (branch likely) the JUMP_FORWARD label could just be SAVE_IP label; superblock generation would then continue from the bytecode at label. That's not something I currently do -- a simplistic approach could do the SAVE_IP followed by EXIT_TRACE, and we can iterate later on continuing from label.

But recording which way branches go is something for a future PR; again the most simplistic thing to do for now is to assume that branching is less likely than not branching (i.e., always generate EXIT_IF_TRUE; POP_TOP for now).

Honestly, for now I think I'll stick to just making the generator explicitly translate JUMPBY(n) into the correct sequence of JUMPBY, STACK_SHRINK and DISPATCH. Nothing should follow JUMPBY then.

Copy link
Member

@markshannon markshannon Jul 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think it makes more sense to consume the condition before exiting

These two instructions, POP_JUMP_IF_TRUE and POP_JUMP_IF_FALSE (the None variants can be broken down into LOAD_CONST None; IS_OP; POP_JUMP...) are special, so don't worry too much about fitting them into the more general framework.

There are a number of approaches, for exits. Here are three:

  1. Leave the condition on the stack, and exit to the POP_JUMP... instruction.
    I don't like this approach as it is inefficient and will make trace stitching tricky
  2. Pop the condition and exit to the target, or successor, instruction.
    This is better, but means that the EXIT uop needs to handle a jump and the exit.
  3. Convert the superblock into a tree, jumping to an exit branch which makes an unconditional exit.
    Definitely my preferred option. Means that there is only one EXIT uop and that it just exits.

Option 3 makes the superblock creation more complex, but simplifies the tier2 interpreter.
We will want to push some operations onto exit branches in the specializer and partial evaluator, so we will need to convert the superblock to a tree at some point.

If option 3 is too complex for this PR, option 2 should work as a temporary step.

So for option 3, using the example above of POP_JUMP_IF_TRUE label we get:

False branch more likely

    POP_JUMP_IF_TRUE_UOP uop_label:
    ...
uop_label:
    SAVE_IP label
    EXIT

True branch more likely

    POP_JUMP_IF_FALSE_UOP uop_label:
    SAVE_IP label
    ...
uop_label:
    EXIT

do { frame->prev_instr += (x); stack_pointer -= (n); goto exit; } while (0)

#undef DEOPT_IF
#define DEOPT_IF(COND, INSTNAME) \
if ((COND)) { \
Expand Down Expand Up @@ -2772,6 +2776,13 @@ _PyUopExecute(_PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject
#define ENABLE_SPECIALIZATION 0
#include "executor_cases.c.h"

case JUMP_TO_TOP:
{
pc = 0;
CHECK_EVAL_BREAKER();
break;
}

case SAVE_IP:
{
frame->prev_instr = ip_offset + oparg;
Expand All @@ -2795,6 +2806,12 @@ _PyUopExecute(_PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject
}
}

exit:
DPRINTF(2, "Jumping!\n");
_PyFrame_SetStackPointer(frame, stack_pointer);
Py_DECREF(self);
return frame;

unbound_local_error:
format_exc_check_arg(tstate, PyExc_UnboundLocalError,
UNBOUNDLOCAL_ERROR_MSG,
Expand Down
2 changes: 2 additions & 0 deletions Python/ceval_macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ GETITEM(PyObject *v, Py_ssize_t i) {
#define JUMPBY(x) (next_instr += (x))
#define SKIP_OVER(x) (next_instr += (x))

#define JUMP_POP_DISPATCH(x, n) do { JUMPBY(x); STACK_SHRINK(n); DISPATCH(); } while (0)

/* OpCode prediction macros
Some opcodes tend to come in pairs thus making it possible to
predict the second code when the first is run. For example,
Expand Down
Loading