GH-115816: Make tier2 optimizer symbols testable, and add a few tests. #115953
Conversation
80066b2 to
0a6fecb
Compare
0a6fecb to
62c544e
Compare
There was a problem hiding this comment.
I realize that I'm arguing with a bunch of code you inherited from @Fidget-Spinner, but while we're moving it around we might as well improve the API.
I also wonder if I'm just looking at a draft PR and you've already figured out in your head what needs to change to make the tests pass, so maybe my review wasn't so useful. Sorry for that. I'm glad you're doing this!
| @@ -33,7 +33,7 @@ dummy_func(void) { | |||
| op(_LOAD_FAST_CHECK, (-- value)) { | |||
| value = GETLOCAL(oparg); | |||
| // We guarantee this will error - just bail and don't optimize it. | |||
| if (sym_is_null(value)) { | |||
| if (_Py_uop_sym_is_null(value)) { | |||
There was a problem hiding this comment.
I find the longer, public names pretty awkward and ugly. I understand they need to be public, but maybe at the top of optimizer_analysis.c we could define shorter aliases with the shorter (unprefixed) names for local use (in optimizer_analysis.c itself and in optimizer_bytecodes.c)? Then the code generator doesn't need to be changed. It would also be simpler to review the important changes in this file.
| @@ -355,18 +355,18 @@ dummy_func(void) { | |||
| // Can determine statically, so we interleave the new locals | |||
| // and make the current stack the new locals. | |||
| // This also sets up for true call inlining. | |||
| if (sym_is_known(self_or_null)) { | |||
| if (_Py_uop_sym_is_null(self_or_null) || _Py_uop_sym_is_not_null(self_or_null)) { | |||
There was a problem hiding this comment.
For example, this is one of the few non-trivial changes in this file.
| PyTypeObject *typ; | ||
| // constant propagated value (might be NULL) | ||
| PyObject *const_val; |
There was a problem hiding this comment.
I'd rename these to type and const. The comment is not particularly useful, but it feels important to clarify the ownership status (once we've figured it out).
| #define OVERALLOCATE_FACTOR 5 | ||
|
|
||
| #define TY_ARENA_SIZE (UOP_MAX_TRACE_LENGTH * OVERALLOCATE_FACTOR) |
There was a problem hiding this comment.
I'd just "inline" OVERALLOCATE_FACTOR, it's only used once (here).
| PyAPI_FUNC(PyObject *) | ||
| _Py_uop_symbols_test(PyObject *self, PyObject *ignored); |
There was a problem hiding this comment.
One line:
| PyAPI_FUNC(PyObject *) | |
| _Py_uop_symbols_test(PyObject *self, PyObject *ignored); | |
| PyAPI_FUNC(PyObject *) _Py_uop_symbols_test(PyObject *self, PyObject *ignored); |
Python/optimizer_symbols.c
Outdated
| } | ||
|
|
||
| _Py_UOpsSymType * | ||
| _Py_uop_sym_new_notnull(_Py_UOpsAbstractInterpContext *ctx) |
There was a problem hiding this comment.
Would be nice if we could agree on whether it's spelled "notnull" (here) or "not_null" (as in _Py_uop_sym_is_not_null).
| } | ||
|
|
||
| bool | ||
| _Py_uop_sym_matches_type(_Py_UOpsSymType *sym, PyTypeObject *typ) |
There was a problem hiding this comment.
An alternative API could be to just get the type, so the caller can do the comparison.
Though I guess in the future we might entertain symbols (other than Bottom/Contradiction) that match multiple types? I'm not sure how an optimizer could make use of that though.
| _Py_uop_ctx_frame_pop( | ||
| _Py_UOpsAbstractInterpContext *ctx | ||
| ) |
|
(Marked it as draft because it obviously is. :-) |
I think improving the API in the same PR is a bad idea. It couples two unrelated changes which slows down development and makes it impossible to revert the changes separately. |
This PR:
_Py_uop_Quite a few tests of the new tests are commented out because they don't pass yet.