gh-131798: JIT: Narrow the return type of isinstance for some known arguments
#133172
Conversation
Python/optimizer_bytecodes.c
Outdated
| // isinstance(obj, cls) where both obj and cls have known types | ||
| // We can deduce either True or False | ||
| PyTypeObject *inst_type = sym_get_type(inst_sym); | ||
| if (sym_matches_type(inst_sym, cls) || PyType_IsSubtype(inst_type, cls)) { |
There was a problem hiding this comment.
This simulates PyObject_TypeCheck
There was a problem hiding this comment.
Can you add a comment to that effect? :)
Python/optimizer_bytecodes.c
Outdated
| @@ -886,6 +886,44 @@ dummy_func(void) { | ||
| } | ||
| } | ||
|
|
||
| op(_CALL_ISINSTANCE, (callable, self_or_null, args[oparg] -- res)) { | ||
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { |
There was a problem hiding this comment.
I've seen this guard used elsewhere with self_or_null but it's not clear to me whether it is needed here as well?
There was a problem hiding this comment.
Yep. It's just a (weird) way of saying "we know whether it's NULL or not". Maybe it's worth adding a helper function (in another PR) to make it clearer, since the meaning is super subtle.
There was a problem hiding this comment.
Cool! Can you also add a test where the second isinstance arg is a class with a metaclass defining __instancecheck__?
class EvenNumberMeta(type):
def __instancecheck__(self, number):
return not number % 2
class EvenNumber(metaclass=EvenNumberMeta):
pass
# Optimizer only narrows to bool, runtime value is True:
even = isinstance(42, EvenNumber)
Lib/test/test_capi/test_opt.py
Outdated
| class Foo: | ||
| bar = 42 | ||
|
|
||
| x = 0 | ||
| for _ in range(n): | ||
| # we only know bar (LOAD_ATTR) is not null (set via sym_new_not_null) | ||
| bar = Foo.bar | ||
| # This will only narrow to bool and not to True due to 'bar' having | ||
| # unknown (non-null) type | ||
| y = isinstance(bar, int) | ||
| if y: | ||
| x += 1 | ||
| return x |
There was a problem hiding this comment.
This is pretty fragile: the class attr lookup is cached in the bytecode, and I actually have a branch I'll open a PR with soon that teaches the optimizer to read these caches.
So instead, let's use everyone's favorite optimization-breaker:
| class Foo: | |
| bar = 42 | |
| x = 0 | |
| for _ in range(n): | |
| # we only know bar (LOAD_ATTR) is not null (set via sym_new_not_null) | |
| bar = Foo.bar | |
| # This will only narrow to bool and not to True due to 'bar' having | |
| # unknown (non-null) type | |
| y = isinstance(bar, int) | |
| if y: | |
| x += 1 | |
| return x | |
| x = 0 | |
| for _ in range(n): | |
| # The optimizer doesn't know the return type here: | |
| bar = eval("42") | |
| # This will only narrow to bool: | |
| y = isinstance(bar, int) | |
| if y: | |
| x += 1 | |
| return x |
There was a problem hiding this comment.
Nice trick to use eval! I updated the test :)
Python/optimizer_bytecodes.c
Outdated
| @@ -886,6 +886,44 @@ dummy_func(void) { | ||
| } | ||
| } | ||
|
|
||
| op(_CALL_ISINSTANCE, (callable, self_or_null, args[oparg] -- res)) { | ||
| if (sym_is_null(self_or_null) || sym_is_not_null(self_or_null)) { |
There was a problem hiding this comment.
Yep. It's just a (weird) way of saying "we know whether it's NULL or not". Maybe it's worth adding a helper function (in another PR) to make it clearer, since the meaning is super subtle.
Python/optimizer_bytecodes.c
Outdated
| // isinstance(obj, cls) where both obj and cls have known types | ||
| // We can deduce either True or False | ||
| PyTypeObject *inst_type = sym_get_type(inst_sym); | ||
| if (sym_matches_type(inst_sym, cls) || PyType_IsSubtype(inst_type, cls)) { |
There was a problem hiding this comment.
Can you add a comment to that effect? :)
Python/optimizer_bytecodes.c
Outdated
| } | ||
| else { | ||
| // isinstance(obj, cls) where obj has unknown type | ||
| res = sym_new_type(ctx, &PyBool_Type); | ||
| } | ||
| } | ||
| else { | ||
| // isinstance(obj, cls) where cls has unknown type | ||
| res = sym_new_type(ctx, &PyBool_Type); | ||
| } | ||
| } | ||
| else { | ||
| res = sym_new_type(ctx, &PyBool_Type); | ||
| } | ||
| } |
There was a problem hiding this comment.
You can avoid all of this repetition by doing an unconditional res = sym_new_type(ctx, &PyBool_Type); at the top, and using sym_set_const(ctx, ...) to narrow it when possible.
There was a problem hiding this comment.
Yup that is way better, updated!
|
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 |
|
(Marking as draft until #133339 is merged which will let us simplify the oparg logic) |
|
I think I addressed all your points. I also added the test you suggested. I'm planning to add support for tuples (e.g. |
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
|
This LGTM. A side note: on PyPy, they can actually narrow |
|
Yeah, could be cool. It's not really useful to us right now, since all of our guards are against exact types/versions, not many subclass checks. |
|
CI failures are unrelated. |
In this PR:
isintance(obj, cls)toTrueif obj is a known type and cls is a known class and obj is a subclass of cls (and vice versa forFalse)isinstancetobool.Brandt also suggested adding an optimization for tuples which I'd like to add in a followup in order to keep the sizes of the individual PRs smaller. Though if you prefer to have it in one PR I can do it as well :)