Skip to content

gh-106581: Split CALL_BOUND_METHOD_EXACT_ARGS into uops #108462

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 1 commit into from
Aug 25, 2023
+171 −35
Merged

gh-106581: Split CALL_BOUND_METHOD_EXACT_ARGS into uops #108462

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 27 additions & 14 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.

10 changes: 10 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.

29 changes: 20 additions & 9 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2940,19 +2940,18 @@ dummy_func(
CHECK_EVAL_BREAKER();
}

// Start out with [NULL, bound_method, arg1, arg2, ...]
// Transform to [callable, self, arg1, arg2, ...]
// Then fall through to CALL_PY_EXACT_ARGS
inst(CALL_BOUND_METHOD_EXACT_ARGS, (unused/1, unused/2, callable, null, unused[oparg] -- unused)) {
op(_CHECK_CALL_BOUND_METHOD_EXACT_ARGS, (callable, null, unused[oparg] -- callable, null, unused[oparg])) {
DEOPT_IF(null != NULL, CALL);
DEOPT_IF(Py_TYPE(callable) != &PyMethod_Type, CALL);
}

op(_INIT_CALL_BOUND_METHOD_EXACT_ARGS, (callable, unused, unused[oparg] -- func, self, unused[oparg])) {
STAT_INC(CALL, hit);
PyObject *self = ((PyMethodObject *)callable)->im_self;
PEEK(oparg + 1) = Py_NewRef(self); // self_or_null
PyObject *meth = ((PyMethodObject *)callable)->im_func;
PEEK(oparg + 2) = Py_NewRef(meth); // callable
self = Py_NewRef(((PyMethodObject *)callable)->im_self);
stack_pointer[-1 - oparg] = self; // Patch stack as it is used by _INIT_CALL_PY_EXACT_ARGS
func = Py_NewRef(((PyMethodObject *)callable)->im_func);
stack_pointer[-2 - oparg] = func; // This is used by CALL, upon deoptimization
Comment on lines +2951 to +2953
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so these lines appear to be entirely redundant for the uops executor, as the stack-effect output of func, self, ... causes the exact same assignments to be emitted automatically right after these lines.

But this is needed for the bytecode interpreter. Since in that case all the uops are squashed together and their inputs and outputs are chained together as local variables, but we really need to update the actual stack here, for the reasons mentioned in the comments.

I don't know how often such uop stack-patching cases will occur (from what I can find, this is the first one?). If there will be more, it might be nice to have syntax to mark an output in the stack-effects definition of the uop as "must actually modify the stack", and then have the cases generator automatically emit this (and the executor cases wouldn't have the duplicate assignments.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the review!

It looks to be pretty uncommon -- calls are special in many ways. Agreed that if this proliferates we should teach the code generator to do this. It also looks like a C compiler would have a hard time optimizing the duplication in Tier 2 away, because there's an intervening Py_DECREF(). If that becomes an issue but it remains limited to just this case we could surround the flushes with #ifdef TIER_ONE / #endif.

I'll merge this and see what's next on the agenda. (I suspect either KW_NAMES or splitting LOAD_ATTR specializations.)

Py_DECREF(callable);
GO_TO_INSTRUCTION(CALL_PY_EXACT_ARGS);
}

op(_CHECK_PEP_523, (--)) {
Expand Down Expand Up @@ -3010,6 +3009,18 @@ dummy_func(
#endif
}

macro(CALL_BOUND_METHOD_EXACT_ARGS) =
unused/1 + // Skip over the counter
_CHECK_PEP_523 +
_CHECK_CALL_BOUND_METHOD_EXACT_ARGS +
_INIT_CALL_BOUND_METHOD_EXACT_ARGS +
_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;

macro(CALL_PY_EXACT_ARGS) =
unused/1 + // Skip over the counter
_CHECK_PEP_523 +
Expand Down
26 changes: 26 additions & 0 deletions Python/executor_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.

98 changes: 87 additions & 11 deletions Python/generated_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.

2 changes: 1 addition & 1 deletion Tools/cases_generator/generate_cases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -444,7 +444,7 @@ def write_metadata(self, metadata_filename: str, pymetadata_filename: str) -> No
with self.out.block("struct opcode_macro_expansion", ";"):
self.out.emit("int nuops;")
self.out.emit(
"struct { int16_t uop; int8_t size; int8_t offset; } uops[8];"
"struct { int16_t uop; int8_t size; int8_t offset; } uops[12];"
)
self.out.emit("")

Expand Down