Repository navigation
gh-130415: Eliminate guards for constant CALL_BUILTIN_O/FAST - #132708
Fidget-Spinner wants to merge 2 commits into
Conversation
|
All commit authors signed the Contributor License Agreement. |
Sorry, something went wrong.
brandtbucher
left a comment
There was a problem hiding this comment.
Some suggestions, but looks good!
Sorry, something went wrong.
| int total_args = oparg; | ||
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { | ||
| total_args += sym_is_not_null(self_or_null); | ||
| // Constant propagate | ||
| if (total_args == 1 && sym_is_const(ctx, callable)) { | ||
| PyObject *callable_o = sym_get_const(ctx, callable); | ||
| if (PyCFunction_CheckExact(callable_o) && | ||
| PyCFunction_GET_FLAGS(callable_o) == METH_O) { | ||
| REPLACE_OP(this_instr, _NOP, 0 ,0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Not sure how much this helps in practice, but in theory we could infer a NULL/non-NULL here based on the oparg, and narrow the type of the callable:
| int total_args = oparg; | |
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { | |
| total_args += sym_is_not_null(self_or_null); | |
| // Constant propagate | |
| if (total_args == 1 && sym_is_const(ctx, callable)) { | |
| PyObject *callable_o = sym_get_const(ctx, callable); | |
| if (PyCFunction_CheckExact(callable_o) && | |
| PyCFunction_GET_FLAGS(callable_o) == METH_O) { | |
| REPLACE_OP(this_instr, _NOP, 0 ,0); | |
| } | |
| } | |
| } | |
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { | |
| int total_args = oparg + sym_is_not_null(self_or_null); | |
| // Constant propagate | |
| if (total_args == 1 && sym_is_const(ctx, callable)) { | |
| PyObject *callable_o = sym_get_const(ctx, callable); | |
| if (PyCFunction_CheckExact(callable_o) && | |
| PyCFunction_GET_FLAGS(callable_o) == METH_O) { | |
| REPLACE_OP(this_instr, _NOP, 0 ,0); | |
| } | |
| } | |
| } | |
| else if (oparg == 0) { | |
| sym_set_non_null(ctx, self_or_null); | |
| } | |
| else { | |
| assert(oparg == 1); | |
| sym_set_null(ctx, self_or_null); | |
| } | |
| sym_set_type(callable, &PyCFunction_Type); |
Sorry, something went wrong.
| op(_GUARD_CALL_BUILTIN_O, (callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) { | ||
| PyObject *callable_o = PyStackRef_AsPyObjectBorrow(callable); | ||
|
|
||
| int total_args = oparg; | ||
| if (!PyStackRef_IsNull(self_or_null)) { | ||
| args--; | ||
| total_args++; | ||
| } | ||
| int total_args = oparg + (!PyStackRef_IsNull(self_or_null)); | ||
| EXIT_IF(total_args != 1); | ||
| EXIT_IF(!PyCFunction_CheckExact(callable_o)); | ||
| EXIT_IF(PyCFunction_GET_FLAGS(callable_o) != METH_O); | ||
| } |
There was a problem hiding this comment.
You might consider breaking this in two: one guard for the number of args, and one guard for callable type/flags. Could be there are situations where you could remove one but not the other.
Speaking of which, is it equally common to have NULL or self for these two instructions? Or is it more likely that it's always one or the other?
It might be worth tightening up the specialization for only one of the cases, since that makes the guards much simpler (and easier to remove). Maybe it's worth running stats.
Sorry, something went wrong.
|
This PR is stale because it has been open for 30 days with no activity. |
Sorry, something went wrong.
|
This PR is attached to gh-130415, which is already completed: the fix for that issue landed through other changes, and this PR's approach was not taken or has since become obsolete. Closing to cull the number of open pull requests. Do not reopen this pull request without getting a go-ahead from a triager or core developer. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.