Skip to content

gh-106581: Split CALL_PY_EXACT_ARGS into uops #107760

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 20 commits into from
Aug 16, 2023
Merged

gh-106581: Split CALL_PY_EXACT_ARGS into uops #107760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
56133bb
Split `CALL_PY_EXACT_ARGS` into uops
gvanrossum Aug 5, 2023
907ff95
Fix merge so it works again (I think)
gvanrossum Aug 9, 2023
2c6be6d
Split into finer-grained uops
gvanrossum Aug 9, 2023
6d78ff2
Fix type error in stacking.py
gvanrossum Aug 10, 2023
0d8e66c
Add test
gvanrossum Aug 10, 2023
b75f30e
Add comment explaining _PUSH_FRAME's unused output effect
gvanrossum Aug 10, 2023
61c2822
Make PUSH_FRAME special case a little less myterious
gvanrossum Aug 10, 2023
f73ea90
Rename Instruction.write to write_case_body
gvanrossum Aug 10, 2023
12910fc
Move next_instr update to a more logical place
gvanrossum Aug 10, 2023
2fafa2c
Don't recompute macro cache offset
gvanrossum Aug 10, 2023
2717b07
Fold and refactor long line in stacking.py
gvanrossum Aug 10, 2023
e487908
Fold long lines in generate_cases.py
gvanrossum Aug 10, 2023
1d549af
Don't emit static assert to executor cases
gvanrossum Aug 10, 2023
f40fb1f
Factor away write_case_body (formerly Instruction.write)
gvanrossum Aug 10, 2023
4f6f8f8
Fold long lines
gvanrossum Aug 11, 2023
6facc8d
Make less of a special case of _PUSH_FRAME
gvanrossum Aug 11, 2023
94630d4
Stop special-casing _PUSH_FRAME altogether
gvanrossum Aug 11, 2023
cf8e2c0
Call _Py_EnterRecursivePy in _FRAME_PUSH
gvanrossum Aug 15, 2023
1e62876
Merge remote-tracking branch 'upstream/main' into call-uops
gvanrossum Aug 15, 2023
05af848
Introduce SAVE_CURRENT_IP uop per Mark's proposal
gvanrossum Aug 16, 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: 43 additions & 4 deletions Include/internal/pycore_opcode_metadata.h
View file
Open in desktop

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

17 changes: 17 additions & 0 deletions Lib/test/test_capi/test_misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2618,6 +2618,23 @@ def testfunc(it):
with self.assertRaises(StopIteration):
next(it)

def test_call_py_exact_args(self):
def testfunc(n):
def dummy(x):
return x+1
for i in range(n):
dummy(i)

opt = _testinternalcapi.get_uop_optimizer()
with temporary_optimizer(opt):
testfunc(10)

ex = get_first_executor(testfunc)
self.assertIsNotNone(ex)
uops = {opname for opname, _, _ in ex}
self.assertIn("_PUSH_FRAME", uops)



if __name__ == "__main__":
unittest.main()
28 changes: 28 additions & 0 deletions Python/abstract_interp_cases.c.h
View file
Open in desktop

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

82 changes: 66 additions & 16 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,13 +957,13 @@ dummy_func(
{
PyGenObject *gen = (PyGenObject *)receiver;
_PyInterpreterFrame *gen_frame = (_PyInterpreterFrame *)gen->gi_iframe;
frame->return_offset = oparg;
STACK_SHRINK(1);
_PyFrame_StackPush(gen_frame, v);
gen->gi_frame_state = FRAME_EXECUTING;
gen->gi_exc_state.previous_item = tstate->exc_info;
tstate->exc_info = &gen->gi_exc_state;
SKIP_OVER(INLINE_CACHE_ENTRIES_SEND);
frame->return_offset = oparg;
DISPATCH_INLINED(gen_frame);
}
if (Py_IsNone(v) && PyIter_Check(receiver)) {
Expand Down Expand Up @@ -996,13 +996,13 @@ dummy_func(
DEOPT_IF(gen->gi_frame_state >= FRAME_EXECUTING, SEND);
STAT_INC(SEND, hit);
_PyInterpreterFrame *gen_frame = (_PyInterpreterFrame *)gen->gi_iframe;
frame->return_offset = oparg;
STACK_SHRINK(1);
_PyFrame_StackPush(gen_frame, v);
gen->gi_frame_state = FRAME_EXECUTING;
gen->gi_exc_state.previous_item = tstate->exc_info;
tstate->exc_info = &gen->gi_exc_state;
SKIP_OVER(INLINE_CACHE_ENTRIES_SEND);
frame->return_offset = oparg;
DISPATCH_INLINED(gen_frame);
}

Expand Down Expand Up @@ -2588,14 +2588,14 @@ dummy_func(
DEOPT_IF(gen->gi_frame_state >= FRAME_EXECUTING, FOR_ITER);
STAT_INC(FOR_ITER, hit);
_PyInterpreterFrame *gen_frame = (_PyInterpreterFrame *)gen->gi_iframe;
frame->return_offset = oparg;
_PyFrame_StackPush(gen_frame, Py_None);
gen->gi_frame_state = FRAME_EXECUTING;
gen->gi_exc_state.previous_item = tstate->exc_info;
tstate->exc_info = &gen->gi_exc_state;
SKIP_OVER(INLINE_CACHE_ENTRIES_FOR_ITER);
assert(next_instr[oparg].op.code == END_FOR ||
next_instr[oparg].op.code == INSTRUMENTED_END_FOR);
frame->return_offset = oparg;
DISPATCH_INLINED(gen_frame);
}

Expand Down Expand Up @@ -2950,32 +2950,72 @@ dummy_func(
GO_TO_INSTRUCTION(CALL_PY_EXACT_ARGS);
}

inst(CALL_PY_EXACT_ARGS, (unused/1, func_version/2, callable, self_or_null, args[oparg] -- unused)) {
ASSERT_KWNAMES_IS_NULL();
op(_CHECK_PEP_523, (--)) {
DEOPT_IF(tstate->interp->eval_frame, CALL);
int argcount = oparg;
if (self_or_null != NULL) {
args--;
argcount++;
}
}

op(_CHECK_FUNCTION_EXACT_ARGS, (func_version/2, callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) {
ASSERT_KWNAMES_IS_NULL();
DEOPT_IF(!PyFunction_Check(callable), CALL);
PyFunctionObject *func = (PyFunctionObject *)callable;
DEOPT_IF(func->func_version != func_version, CALL);
PyCodeObject *code = (PyCodeObject *)func->func_code;
DEOPT_IF(code->co_argcount != argcount, CALL);
DEOPT_IF(code->co_argcount != oparg + (self_or_null != NULL), CALL);
}

op(_CHECK_STACK_SPACE, (callable, unused, unused[oparg] -- callable, unused, unused[oparg])) {
PyFunctionObject *func = (PyFunctionObject *)callable;
PyCodeObject *code = (PyCodeObject *)func->func_code;
DEOPT_IF(!_PyThreadState_HasStackSpace(tstate, code->co_framesize), CALL);
}

op(_INIT_CALL_PY_EXACT_ARGS, (callable, self_or_null, args[oparg] -- new_frame: _PyInterpreterFrame*)) {
int argcount = oparg;
if (self_or_null != NULL) {
args--;
argcount++;
}
STAT_INC(CALL, hit);
_PyInterpreterFrame *new_frame = _PyFrame_PushUnchecked(tstate, func, argcount);
PyFunctionObject *func = (PyFunctionObject *)callable;
new_frame = _PyFrame_PushUnchecked(tstate, func, argcount);
for (int i = 0; i < argcount; i++) {
new_frame->localsplus[i] = args[i];
}
// Manipulate stack directly since we leave using DISPATCH_INLINED().
STACK_SHRINK(oparg + 2);
SKIP_OVER(INLINE_CACHE_ENTRIES_CALL);
}

// The 'unused' output effect represents the return value
// (which will be pushed when the frame returns).
// It is needed so CALL_PY_EXACT_ARGS matches its family.
op(_PUSH_FRAME, (new_frame: _PyInterpreterFrame* -- unused)) {
Copy link
Member

@markshannon markshannon Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Since _PUSH_FRAME is just frame->return_offset = 0; DISPATCH_INLINED(new_frame);, it would make sense to spell out DISPATCH_INLINED to clarify which operations that need to be different for tier 1 and tier 2.
Something like:

op(_PUSH_FRAME, (new_frame: _PyInterpreterFrame* -- unused)) {
    SAVE_FRAME_STATE();   // Equivalent to frame->prev_instr = next_instr - 1; _PyFrame_SetStackPointer(frame, stack_pointer);
    frame->return_offset = 0;
    new_frame->previous = frame;
    frame = cframe.current_frame = new_frame;
    CALL_STAT_INC(inlined_py_calls);
    if (_Py_EnterRecursivePy(tstate)) {
        goto exit_unwind;
    }
    START_FRAME(); // Equivalent to next_instr = frame->prev_instr  + 1; stack_pointer = 
            stack_pointer = _PyFrame_GetStackPointer(frame);
}

Copy link
Member

@markshannon markshannon Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For example: 2b3e6f2

In which case the code generators needs to know to push the temporary stack values to the real stack before SAVE_FRAME_STATE()

Copy link
Member Author

Choose a reason for hiding this comment

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

I have to study this more. A problem is that the Tier 1 and Tier 2 versions of _PUSH_FRAME are so different. I am working on a mechanism to be able to say

#if TIER_ONE
<code for Tier 1>
#else
<code for Tier 2>
#endif

I'm not sure yet what you mean with your last remark about pushing temp stack values.

Copy link
Member Author

@gvanrossum gvanrossum Aug 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Comparing carefully the two versions of DISPATCH_INLINED (adding frame->return_offset = 0 which precedes it in both cases):

In Tier 1:

        frame->return_offset = 0;
        assert(tstate->interp->eval_frame == NULL);
        _PyFrame_SetStackPointer(frame, stack_pointer);
        frame->prev_instr = next_instr - 1;
        (NEW_FRAME)->previous = frame;
        frame = cframe.current_frame = (NEW_FRAME);
        CALL_STAT_INC(inlined_py_calls);
        goto start_frame;

In Tier 2:

        frame->return_offset = 0;
        assert(tstate->interp->eval_frame == NULL);
        _PyFrame_SetStackPointer(frame, stack_pointer);
        frame->prev_instr -= 1;
        (NEW_FRAME)->previous = frame;
        frame = tstate->cframe->current_frame = (NEW_FRAME);
        CALL_STAT_INC(inlined_py_calls);
        stack_pointer = _PyFrame_GetStackPointer(frame);
        ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive;

Diff:

@@ -1,8 +1,9 @@
         frame->return_offset = 0;
         assert(tstate->interp->eval_frame == NULL);
         _PyFrame_SetStackPointer(frame, stack_pointer);
-        frame->prev_instr = next_instr - 1;
+        frame->prev_instr -= 1;
         (NEW_FRAME)->previous = frame;
-        frame = cframe.current_frame = (NEW_FRAME);
+        frame = tstate->cframe->current_frame = (NEW_FRAME);
         CALL_STAT_INC(inlined_py_calls);
-        goto start_frame;
+        stack_pointer = _PyFrame_GetStackPointer(frame);
+        ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive;

Note that the Tier 2 version must be preceded by a SAVE_IP, which does the equivalent of frame->prev_instr = next_instr. If we had a Tier 1 version of SAVE_IP we could include it in the macro definition:

        macro(CALL_PY_EXACT_ARGS) =
            unused/1 + // Skip over the counter
            _CHECK_PEP_523 +
            _CHECK_FUNCTION_EXACT_ARGS +
            _CHECK_STACK_SPACE +
            _INIT_CALL_PY_EXACT_ARGS +
            SAVE_IP +      // <-------------- added
            _PUSH_FRAME;

which would reduce the special-casing in the code generator a bit (it would still need to do something special for SAVE_IP to ensure that its oparg has the right value, different from the oparg of the macro (which is the argument count). This would take care of the first diff chunk (what to assign to frame->prev_inst), but it would still be pretty fragile. (Like my current version, it would entice the optimizer to incorrectly try to remove the SAVE_IP uop.)

The second diff chunk relates to how we set cframe.current_frame -- in Tier 2 we must access this through the tstate.

The third and final diff chunk relates to really start using the new frame. In Tier 1, this must actually do the following:

  • Check recursion
  • Load stack_pointer
  • Load next_instr
  • Dispatch to the next opcode.

This is done by the code at start_frame.

In Tier 2 there is no start_frame label (the only uop that can go to a label is EXIT_TRACE, and of course DEOPT_IF and ERROR_IF also jump). So we load stack_frame here. There is no direct equivalent to next_instr, but we have to set ip_offset, which SAVE_IP adds to its oparg to get the prev_instr value. (This variable is a cache for frame->code->co_code_adaptive, to save some memory loads, so whenever frame changes we must update it.)

(More later.)

Copy link
Member Author

Choose a reason for hiding this comment

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

There's another thing though, and I think that is what Mark meant. In Tier 1 the code generation for macros is special-cased for _PUSH_FRAME so that both the stack adjustment and the next_instr adjustment are emitted before the _PUSH_FRAME opcode. This is done so that the flushing of these variables to the frame in the DISPATCH_INLINED macro flush the correct values.

But this is really ugly and unprincipled, and the logic is much hairier than the other special cases for _PUSH_FRAME. One of Mark's ideas here is to make this special case look for uops using the SAVE_FRAME_STATE macro rather than for the specific uop _PUSH_FRAME. But detecting when to trigger the special case is only part of the problem -- IMO the worse problem is that the special case itself is so ugly:

        dispatch_inlined_special_case = False
        if mgr is managers[-1] and mgr.instr.always_exits.startswith("DISPATCH_INLINED") and mgr.instr.name == "_PUSH_FRAME":
            dispatch_inlined_special_case = True
            temp = mgr.final_offset.clone()
            temp.deeper(StackEffect(UNUSED))  # Hack
            out.stack_adjust(temp.deep, temp.high)
            # Use clone() since adjust_inverse() mutates final_offset
            mgr.adjust_inverse(mgr.final_offset.clone())
            if cache_adjust:
                out.emit(f"next_instr += {cache_adjust};")

The last 4 lines here, starting with # Use clone(), occur further down too, for the normal case (after the final uop). I don't even recall why the temp.deeper() call is needed!

I'll mull this over some more.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I have addressed this. @markshannon Please have another look. Assuming the tests look okay I'll un-draft this.

// Write it out explicitly because it's subtly different.
// Eventually this should be the only occurrence of this code.
frame->return_offset = 0;
DISPATCH_INLINED(new_frame);
assert(tstate->interp->eval_frame == NULL);
_PyFrame_SetStackPointer(frame, stack_pointer);
new_frame->previous = frame;
CALL_STAT_INC(inlined_py_calls);
#if TIER_ONE
frame = cframe.current_frame = new_frame;
goto start_frame;
#endif
#if TIER_TWO
frame = tstate->cframe->current_frame = new_frame;
ERROR_IF(_Py_EnterRecursivePy(tstate), exit_unwind);
stack_pointer = _PyFrame_GetStackPointer(frame);
ip_offset = (_Py_CODEUNIT *)_PyFrame_GetCode(frame)->co_code_adaptive;
#endif
}

macro(CALL_PY_EXACT_ARGS) =
unused/1 + // Skip over the counter
_CHECK_PEP_523 +
_CHECK_FUNCTION_EXACT_ARGS +
_CHECK_STACK_SPACE +
_INIT_CALL_PY_EXACT_ARGS +
SAVE_IP + // Tier 2 only; special-cased oparg
SAVE_CURRENT_IP + // Sets frame->prev_instr
_PUSH_FRAME;

inst(CALL_PY_WITH_DEFAULTS, (unused/1, func_version/2, callable, self_or_null, args[oparg] -- unused)) {
ASSERT_KWNAMES_IS_NULL();
DEOPT_IF(tstate->interp->eval_frame, CALL);
Expand Down Expand Up @@ -3736,6 +3776,16 @@ dummy_func(
frame->prev_instr = ip_offset + oparg;
}

op(SAVE_CURRENT_IP, (--)) {
#if TIER_ONE
frame->prev_instr = next_instr - 1;
#endif
#if TIER_TWO
// Relies on a preceding SAVE_IP
frame->prev_instr--;
#endif
}

op(EXIT_TRACE, (--)) {
frame->prev_instr--; // Back up to just before destination
_PyFrame_SetStackPointer(frame, stack_pointer);
Expand Down
6 changes: 1 addition & 5 deletions Python/ceval.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -602,11 +602,6 @@ int _Py_CheckRecursiveCallPy(
return 0;
}

static inline int _Py_EnterRecursivePy(PyThreadState *tstate) {
return (tstate->py_recursion_remaining-- <= 0) &&
_Py_CheckRecursiveCallPy(tstate);
}


static inline void _Py_LeaveRecursiveCallPy(PyThreadState *tstate) {
tstate->py_recursion_remaining++;
Expand Down Expand Up @@ -770,6 +765,7 @@ _PyEval_EvalFrameDefault(PyThreadState *tstate, _PyInterpreterFrame *frame, int
#endif
{

#define TIER_ONE 1
#include "generated_cases.c.h"

/* INSTRUMENTED_LINE has to be here, rather than in bytecodes.c,
Expand Down
5 changes: 5 additions & 0 deletions Python/ceval_macros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,3 +364,8 @@ static const convertion_func_ptr CONVERSION_FUNCTIONS[4] = {
#else
#define _Py_atomic_load_relaxed_int32(ATOMIC_VAL) _Py_atomic_load_relaxed(ATOMIC_VAL)
#endif

static inline int _Py_EnterRecursivePy(PyThreadState *tstate) {
return (tstate->py_recursion_remaining-- <= 0) &&
_Py_CheckRecursiveCallPy(tstate);
}
2 changes: 2 additions & 0 deletions Python/executor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ _PyUopExecute(_PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject
OBJECT_STAT_INC(optimization_uops_executed);
switch (opcode) {

#define TIER_TWO 2
#include "executor_cases.c.h"

default:
Expand All @@ -106,6 +107,7 @@ _PyUopExecute(_PyExecutorObject *executor, _PyInterpreterFrame *frame, PyObject
pop_2_error:
STACK_SHRINK(1);
pop_1_error:
pop_1_exit_unwind:
STACK_SHRINK(1);
error:
// On ERROR_IF we return NULL as the frame.
Expand Down
Loading