GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — gh-130415: Eliminate guards for constant CALL_BUILTIN_O/FAST by Fidget-Spinner · Pull Request #132708 · python/cpython Conversation
|
|
All commit authors signed the Contributor License Agreement. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
brandtbucher
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions, but looks good!
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| 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); | ||
| } |
brandtbucher
Apr 24, 2025
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
This PR is stale because it has been open for 30 days with no activity. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
3 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.