Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 33.9k
gh-130415: Narrowing to constants in branches involving is comparisons with a constant#143895
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-130415: Narrowing to constants in branches involving is comparisons with a constant #143895
Conversation
reidenong commented Jan 15, 2026 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
Fidget-Spinner commented Jan 15, 2026
This is pretty cool! Thanks for doing this, I'll review it soon. |
markshannon 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.
I like the idea, it provides some more reasoning about values and fits nicely into the existing framework.
Thanks for doing this.
I would recommend changing the construction of predicates to be more relaxed and only check for constants when the predicate is resolved. It would be more powerful, easier to reason about and maybe faster.
As an example:
p0=aisbp1=bisNoneexit_if(notp1) exit_if(notp0) # a must be NoneBy restricting p0 to cases where a is a const or b is a const, we cannot infer that a is None after the second exit.
Include/internal/pycore_optimizer.h Outdated
| externJitOptRef_Py_uop_sym_new_compact_int(JitOptContext*ctx); | ||
| externvoid_Py_uop_sym_set_compact_int(JitOptContext*ctx, JitOptRefsym); | ||
| externJitOptRef_Py_uop_sym_new_predicate(JitOptContext*ctx, JitOptRefsubject_ref, JitOptRefconstant_ref, JitOptPredicateKindkind, boolinvert); | ||
| externbool_Py_uop_sym_is_known_singleton(JitOptContext*ctx, JitOptRefsym); |
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.
It doesn't matters whether something is a singleton or not, so you can drop this.
| typedefenum{ | ||
| JIT_PRED_IS, | ||
| // JIT_PRED_EQ, |
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.
Either remove this, or add a TODO note that we'll be adding EQ, NE, etc.
Python/optimizer_bytecodes.c Outdated
| op(_IS_OP, (left, right--b, l, r)){ | ||
| b=sym_new_type(ctx, &PyBool_Type); | ||
| boolinvert= (oparg!=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.
It would be both more powerful and result in cleaner code, if we left all analysis until the predicate is resolved.
We will often have more information then.
Python/optimizer_symbols.c Outdated
| return; | ||
| caseJIT_SYM_PREDICATE_TAG: | ||
| if (!PyBool_Check(const_val) || | ||
| (_Py_uop_sym_is_const(ctx, ref) && |
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.
Drop these extra checks. I'm not sure they are necessary and may be creating a contradiction where none exists.
If const_val is a boolean, then we can resolve the predicate, sym_apply_predicate_narrowing(ctx, flag, as_bool(const_val))
Python/optimizer_symbols.c Outdated
| } | ||
| JitOptRef | ||
| _Py_uop_sym_new_predicate(JitOptContext*ctx, JitOptRefsubject_ref, JitOptRefconstant_ref, JitOptPredicateKindkind, boolinvert) |
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.
We don't need to restrict this to the case where one of the values is a constant. If a is b is true, then a and b refer to the same object regardless of whether one is constant.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
- Delay analysis until predicate resolves - Generalize predicate to lhs, rhs - Expand to accept all safe constants (not just singletons)
reidenong commented Jan 16, 2026
Hi @markshannon I think I've addressed the stated problems, a summary of changes is that
Additionally, you mentioned that for Another implementation I initially considered would be to move the Thanks! |
Fidget-Spinner commented Jan 16, 2026
We need a test in the optimizer_symbols.c file (at the bottom, the last function). Also CI is failing, can you please investigate? Thanks! |
python-cla-botbot commented Jan 16, 2026 • 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.
Fidget-Spinner commented Jan 16, 2026
CI failing due to #142911 |
reidenong commented Jan 16, 2026
ok, will take a look and get back asap |
Fidget-Spinner commented Jan 17, 2026
@reidenong you should be able to pull in main now and it will build. |
reidenong commented Jan 17, 2026
@Fidget-Spinner I have added the unit tests, ptal. thanks!
btw it builds now but what was the issue? I'm a bit confused on what happened. |
reidenong commented Jan 19, 2026
BTW is there a need to update the symbol-lattice diagram in |
- `v is 1` is not recommended in Python
markshannon 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.
This looks correct, and thanks for updating the ascii art.
I've a couple of suggestions that should help long term maintenance, otherwise LGTM.
Include/internal/pycore_optimizer.h Outdated
| externbool_Py_uop_sym_is_compact_int(JitOptRefsym); | ||
| externJitOptRef_Py_uop_sym_new_compact_int(JitOptContext*ctx); | ||
| externvoid_Py_uop_sym_set_compact_int(JitOptContext*ctx, JitOptRefsym); | ||
| externJitOptRef_Py_uop_sym_new_predicate(JitOptContext*ctx, JitOptReflhs_ref, JitOptRefrhs_ref, JitOptPredicateKindkind, boolinvert); |
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.
Now that the predicate is symmetrical (meaning that we treat the lhs and rhs the same, not that it is commutative), I think we should drop the invert parameter and field and only use the kind, adding an IS_NOT kind.
Python/optimizer_symbols.c Outdated
| boollhs_is_const=_Py_uop_sym_is_safe_const(ctx, lhs_ref); | ||
| boolrhs_is_const=_Py_uop_sym_is_safe_const(ctx, rhs_ref); | ||
| if (pred.kind==JIT_PRED_IS&& (lhs_is_const||rhs_is_const)){ |
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.
Can you use a switch, that way the compiler will complain about missing cases as we add more kinds.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
reidenong commented Jan 19, 2026
I have made the requested changes; please review again |
Thanks for making the requested changes! @markshannon: please review the changes made to this pull request. |
Fidget-Spinner commented Jan 21, 2026
Please fix the merge conflicts, thank you! |
markshannon 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.
This looks good now. Thanks for doing this.
0b08438 into python:mainUh oh!
There was an error while loading. Please reload this page.
This PR does constant narrowing in the JIT optimizer for branches with _IS_OP comparisons with the constant singletons True, False, None.
It introduces a new optimizer symbol, predicate, which describes a relationship between two objects. We create a new optimizer symbol upon encountering a comparison with a constant, and when we have definitive information on a object after a branch is taken (or not taken) we narrow it to a constant. Ideally, this new symbol will be easily extendable for other ops like (==, !=, contains etc.).
Unit tests intend to show constant narrowing by performing narrowing, then constant folding with other ops and showing the proof of the latter.
All feedback is welcome, with emphasis on the overall approach, how this new symbol fits into the lattice, and naming.
Thanks in advance.