gh-130415: Narrowing to constants in branches involving is comparisons with a constant #143895
Conversation
|
This is pretty cool! Thanks for doing this, I'll review it soon. |
markshannon
left a comment
There was a problem hiding this comment.
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 = a is b
p1 = b is None
exit_if(not p1)
exit_if(not p0)
# 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.
| extern JitOptRef _Py_uop_sym_new_compact_int(JitOptContext *ctx); | ||
| extern void _Py_uop_sym_set_compact_int(JitOptContext *ctx, JitOptRef sym); | ||
| extern JitOptRef _Py_uop_sym_new_predicate(JitOptContext *ctx, JitOptRef subject_ref, JitOptRef constant_ref, JitOptPredicateKind kind, bool invert); | ||
| extern bool _Py_uop_sym_is_known_singleton(JitOptContext *ctx, JitOptRef sym); |
There was a problem hiding this comment.
It doesn't matters whether something is a singleton or not, so you can drop this.
|
|
||
| typedef enum { | ||
| JIT_PRED_IS, | ||
| // JIT_PRED_EQ, |
There was a problem hiding this comment.
Either remove this, or add a TODO note that we'll be adding EQ, NE, etc.
|
|
||
| op(_IS_OP, (left, right -- b, l, r)) { | ||
| b = sym_new_type(ctx, &PyBool_Type); | ||
| bool invert = (oparg != 0); |
There was a problem hiding this comment.
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.
| return; | ||
| case JIT_SYM_PREDICATE_TAG: | ||
| if (!PyBool_Check(const_val) || | ||
| (_Py_uop_sym_is_const(ctx, ref) && |
There was a problem hiding this comment.
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))
| } | ||
|
|
||
| JitOptRef | ||
| _Py_uop_sym_new_predicate(JitOptContext *ctx, JitOptRef subject_ref, JitOptRef constant_ref, JitOptPredicateKind kind, bool invert) |
There was a problem hiding this comment.
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 |
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.