gh-131798: Split CHECK_FUNCTION_VERSION into two guards in the JIT#145080
gh-131798: Split CHECK_FUNCTION_VERSION into two guards in the JIT#145080Sacul0457 wants to merge 11 commits intopython:mainfrom
CHECK_FUNCTION_VERSION into two guards in the JIT#145080Conversation
37e58ef to
e775a6b
Compare
|
Is this optimization still correct/sound after #146101? |
| op(_CHECK_FUNCTION_VERSION, (func_version/2, callable, self_or_null, unused[oparg] -- callable, self_or_null, unused[oparg])) { | ||
| assert(sym_matches_type(callable, &PyFunction_Type)); | ||
| if (sym_get_func_version(callable) == func_version) { | ||
| REPLACE_OP(this_instr, _NOP, 0, 0); |
There was a problem hiding this comment.
I just realised this existing code I wrote might be wrong
The idea is that it's possible to modify a function version midway in a trace, so we need to recheck it or at least add a watcher at the first check.
There was a problem hiding this comment.
@Sacul0457 would you be willing to help me fix this (please open a separate issue if so)? Basically you'd need to add a PyFunction_AddWatcher in the else case of this branch. (See what we do in _GUARD_TYPE_VERSION). It's just a callback that invalidates the executors (see type_watcher_callback).
There was a problem hiding this comment.
Sure, Ill open an issue and the PR in a bit
There was a problem hiding this comment.
Hi, I was just taking a look at this issue and I've got a few questions:
- We add a
PyFunction_AddWatcheronly if thefunc_versionwas set properly, like what_GUARD_TYPE_VERSIONversion does? - If so, what
PyFunction_WatchCallbackdo I add toPyFunction_AddWatcheror am I suppose to create one similar totype_watcher_callback
I'll continue to look into this in the meantime, thx! :)
There was a problem hiding this comment.
We add a PyFunction_AddWatcher only if the func_version was set properly, like what _GUARD_TYPE_VERSION version does?
Yup!
If so, what PyFunction_WatchCallback do I add to PyFunction_AddWatcher or am I suppose to create one similar to type_watcher_callback
Yes you need to create your own one with its own ID similar to type_watcher_callback.
There was a problem hiding this comment.
The function watcher needs to be more specific though: we want to invalidate for everything except for PyFunction_EVENT_DESTROY. As that doesn't actually invalidate any of our assumptions.
There was a problem hiding this comment.
Ah ok ty!
Yes you need to create your own one with its own ID similar to type_watcher_callback.
Just one more thing on the callback (sorry!),
type_watcher_callback calls PyType_Unwatch, and so I'm guessing I also need to create some sort of PyFunc_Unwatch?
There was a problem hiding this comment.
It should be PyFunction_ClearWatcher (check Objects/funcobject.c)
This PR splits up
_CHECK_FUNCTION_VERSIONinto_GUARD_CALLABLE_FUNCTIONand itself. This allows the optimizer to remove thePyFunction_Checkcheck if we know the type. I think the naming can be improved but this is just what I went for.This can be done for
_CHECK_FUNCTION_VERSION_KWtoo but I left that out to keep the change smaller.