gh-131798: JIT: Further optimize _CALL_ISINSTANCE for class tuples
#134543
Conversation
tomasr8
changed the title
_CALL_ISINSTANCE for class tuples_CALL_ISINSTANCE for class tuples
| self.assertIn("_BUILD_TUPLE", uops) | ||
| self.assertIn("_POP_CALL_TWO_LOAD_CONST_INLINE_BORROW", uops) |
There was a problem hiding this comment.
_BUILD_TUPLE is preventing us from optimizing out _POP_CALL_TWO_LOAD_CONST_INLINE_BORROW.
The bytecode is basically:
LOAD_CONST
LOAD_CONST
_BUILD_TUPLE
_POP_CALL_TWO_LOAD_CONST_INLINE_BORROWTo optimize this, we'd need some special handling for _BUILD_TUPLE in remove_unneeded_uops.
| @@ -938,6 +938,9 @@ dummy_func(void) { | |||
| } | |||
|
|
|||
| op(_CALL_ISINSTANCE, (unused, unused, instance, cls -- res)) { | |||
| // The below define is equivalent to PyObject_TypeCheck(inst, cls) | |||
| #define sym_IS_SUBTYPE(inst, cls) ((inst) == (cls) || PyType_IsSubtype(inst, cls)) | |||
There was a problem hiding this comment.
I'm not sure about this define, maybe it's fine to duplicate this logic?
There was a problem hiding this comment.
I'll let you choose. We should probably either duplicate it, or just make it a proper function in optimizer_symbols.c.
| if (!sym_has_type(item)) { | ||
| // There is an unknown item in the tuple, | ||
| // we can no longer deduce False. | ||
| all_items_known = false; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We can't infer anything in this case, since the class could do anything in its __instancecheck__ or whatever, so it's no longer side-effect free. Need to bail on the whole optimization at this point.
So I don't think we need all_items_known, either. We can either:
- Break early on our first
True, like you do below, and inferTrue. - Loop over everything and infer
False. - Bail on an unknown thing and infer
bool.
There was a problem hiding this comment.
I guess we could still do something like this if we know sym_get_type(item) == &PyType_Type, so it's guaranteed side-effect-free, we just don't know the result of the test. But that seems like a rare case (knowing something is a type, but not which type it actually is).
| @@ -938,6 +938,9 @@ dummy_func(void) { | |||
| } | |||
|
|
|||
| op(_CALL_ISINSTANCE, (unused, unused, instance, cls -- res)) { | |||
| // The below define is equivalent to PyObject_TypeCheck(inst, cls) | |||
| #define sym_IS_SUBTYPE(inst, cls) ((inst) == (cls) || PyType_IsSubtype(inst, cls)) | |||
There was a problem hiding this comment.
I'll let you choose. We should probably either duplicate it, or just make it a proper function in optimizer_symbols.c.
|
When you're done making the requested changes, leave the comment: |
We can already const eval
isinstance(inst, cls)calls when both arguments are known, but only ifclsis a single class (e.g.int).This PR adds support for
isinstance(inst, (cls1, cls2, ..., clsN)). This allows us to optimize for example:isinstance(42, (int, str))(toTrue)isinstance(42, (bool, str))(toFalse)We can narrow to
Trueeven when only some of the classes are known, as long asinstis an instance of one of the known classes.