gh-130415: Eliminate guards for constant CALL_BUILTIN_O/FAST #132708
+515
−247
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Fidget-Spinner
commented
Apr 18, 2025
•
- Issue: Better constant narrowing in the JIT optimizer #130415
|
All commit authors signed the Contributor License Agreement. |
17 tasks
brandtbucher
approved these changes
Apr 24, 2025
Comment on lines
+690
to
+701
| 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:
Suggested change
| 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); |
Comment on lines
+4115
to
+4121
| 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.