Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-130415: Eliminate guards for constant CALL_BUILTIN_O/FAST#132708
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
python-cla-botbot commentedApr 18, 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.
All commit authors signed the Contributor License Agreement. |
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!
| 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 aNULL/non-NULL here based on the oparg, and narrow the type of the callable:
| inttotal_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)) { | |
| inttotal_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); | |
| } | |
| } | |
| } | |
| elseif (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); |
| 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); | ||
| } |
brandtbucherApr 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 haveNULL orself 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.
Uh oh!
There was an error while loading.Please reload this page.