Skip to content

GH-115816: Make tier2 optimizer symbols testable, and add a few tests. #115953

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Feb 27, 2024
Merged

GH-115816: Make tier2 optimizer symbols testable, and add a few tests. #115953

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
63d087c
Factor out T2 optimizer symbols for easier testing
markshannon Feb 22, 2024
bc4583c
Add _Py_uop_ prefix to all T2 symbols function names
markshannon Feb 22, 2024
11b0e43
Add test for T2 symbols
markshannon Feb 22, 2024
b10960f
Add back underscores
markshannon Feb 22, 2024
62c544e
Add new file
markshannon Feb 22, 2024
2b3a72b
Fix typo
markshannon Feb 22, 2024
c656141
Fix whitespace
markshannon Feb 22, 2024
694d7aa
Merge branch 'main' into make-symbols-testable
markshannon Feb 22, 2024
7e054bc
Swap top and bottom to better match standard type theory.
markshannon Feb 22, 2024
91ad8ab
Add missing underscore
markshannon Feb 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 85 additions & 0 deletions Include/internal/pycore_optimizer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ extern "C" {
#endif

#include "pycore_uop_ids.h"
#include <stdbool.h>

// This is the length of the trace we project initially.
#define UOP_MAX_TRACE_LENGTH 512
Expand All @@ -25,6 +26,90 @@ extern PyTypeObject _PyDefaultOptimizer_Type;
extern PyTypeObject _PyUOpExecutor_Type;
extern PyTypeObject _PyUOpOptimizer_Type;

/* Symbols */

struct _Py_UOpsSymType {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference would be to name this _Py_UopsSymbol.

int flags;
PyTypeObject *typ;
// constant propagated value (might be NULL)
PyObject *const_val;
Comment on lines +33 to +35
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

};

// Holds locals, stack, locals, stack ... co_consts (in that order)
#define MAX_ABSTRACT_INTERP_SIZE 4096

#define OVERALLOCATE_FACTOR 5

#define TY_ARENA_SIZE (UOP_MAX_TRACE_LENGTH * OVERALLOCATE_FACTOR)
Comment on lines +41 to +43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd just "inline" OVERALLOCATE_FACTOR, it's only used once (here).


// Need extras for root frame and for overflow frame (see TRACE_STACK_PUSH())
#define MAX_ABSTRACT_FRAME_DEPTH (TRACE_STACK_SIZE + 2)

typedef struct _Py_UOpsSymType _Py_UOpsSymType;

struct _Py_UOpsAbstractFrame {
// Max stacklen
int stack_len;
int locals_len;

_Py_UOpsSymType **stack_pointer;
_Py_UOpsSymType **stack;
_Py_UOpsSymType **locals;
};

typedef struct _Py_UOpsAbstractFrame _Py_UOpsAbstractFrame;

typedef struct ty_arena {
int ty_curr_number;
int ty_max_number;
_Py_UOpsSymType arena[TY_ARENA_SIZE];
} ty_arena;
Comment on lines +62 to +66
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The naming of this struct, type, and its members could use some improvements.


struct _Py_UOpsAbstractInterpContext {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just _Py_UOpsContext?

PyObject_HEAD
// The current "executing" frame.
_Py_UOpsAbstractFrame *frame;
_Py_UOpsAbstractFrame frames[MAX_ABSTRACT_FRAME_DEPTH];
int curr_frame_depth;

// Arena for the symbolic types.
ty_arena t_arena;

_Py_UOpsSymType **n_consumed;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This name is misleading (n_foobar would imply it's the number of foobars, but it's not -- it's a pointer to the next unused foobar in the array of foobars). Both this name and the next (limit, being a bit too general) could use a better name, and possibly a better interface (why not express these as indexes instead of pointers?).

_Py_UOpsSymType **limit;
_Py_UOpsSymType *locals_and_stack[MAX_ABSTRACT_INTERP_SIZE];
};

typedef struct _Py_UOpsAbstractInterpContext _Py_UOpsAbstractInterpContext;

extern bool _Py_uop_sym_is_null(_Py_UOpsSymType *sym);
extern bool _Py_uop_sym_is_not_null(_Py_UOpsSymType *sym);
extern bool _Py_uop_sym_is_const(_Py_UOpsSymType *sym);
extern PyObject *_Py_uop_sym_get_const(_Py_UOpsSymType *sym);
extern _Py_UOpsSymType *_Py_uop_sym_new_unknown(_Py_UOpsAbstractInterpContext *ctx);
extern _Py_UOpsSymType *_Py_uop_sym_new_not_null(_Py_UOpsAbstractInterpContext *ctx);
extern _Py_UOpsSymType *_Py_uop_sym_new_type(
_Py_UOpsAbstractInterpContext *ctx, PyTypeObject *typ);
extern _Py_UOpsSymType *_Py_uop_sym_new_const(_Py_UOpsAbstractInterpContext *ctx, PyObject *const_val);
extern _Py_UOpsSymType *_Py_uop_sym_new_null(_Py_UOpsAbstractInterpContext *ctx);
extern bool _Py_uop_sym_matches_type(_Py_UOpsSymType *sym, PyTypeObject *typ);
extern void _Py_uop_sym_set_null(_Py_UOpsSymType *sym);
extern void _Py_uop_sym_set_type(_Py_UOpsSymType *sym, PyTypeObject *tp);

extern int _Py_uop_abstractcontext_init(_Py_UOpsAbstractInterpContext *ctx);
extern void _Py_uop_abstractcontext_fini(_Py_UOpsAbstractInterpContext *ctx);

extern _Py_UOpsAbstractFrame *_Py_uop_ctx_frame_new(
_Py_UOpsAbstractInterpContext *ctx,
PyCodeObject *co,
_Py_UOpsSymType **localsplus_start,
int n_locals_already_filled,
int curr_stackentries);
extern int _Py_uop_ctx_frame_pop(_Py_UOpsAbstractInterpContext *ctx);

PyAPI_FUNC(PyObject *)
_Py_uop_symbols_test(PyObject *self, PyObject *ignored);
Comment on lines +110 to +111
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One line:

Suggested change
PyAPI_FUNC(PyObject *)
_Py_uop_symbols_test(PyObject *self, PyObject *ignored);
PyAPI_FUNC(PyObject *) _Py_uop_symbols_test(PyObject *self, PyObject *ignored);


#ifdef __cplusplus
}
#endif
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_generated_cases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ def test_overridden_abstract_args(self):

case OP2: {
_Py_UOpsSymType *out;
out = sym_new_unknown(ctx);
out = _Py_uop_sym_new_unknown(ctx);
if (out == NULL) goto out_of_space;
stack_pointer[-1] = out;
break;
Expand All @@ -925,7 +925,7 @@ def test_no_overridden_case(self):
output = """
case OP: {
_Py_UOpsSymType *out;
out = sym_new_unknown(ctx);
out = _Py_uop_sym_new_unknown(ctx);
if (out == NULL) goto out_of_space;
stack_pointer[-1] = out;
break;
Expand Down
7 changes: 7 additions & 0 deletions Lib/test/test_optimizer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,5 +77,12 @@ def func(x=0):
_testinternalcapi.get_rare_event_counters()["func_modification"]
)


class TestOptimizerSymbols(unittest.TestCase):

def test_optimizer_symbols(self):
_testinternalcapi.uop_symbols_test()


if __name__ == "__main__":
unittest.main()
1 change: 1 addition & 0 deletions Makefile.pre.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ PYTHON_OBJS= \
Python/object_stack.o \
Python/optimizer.o \
Python/optimizer_analysis.o \
Python/optimizer_symbols.o \
Python/parking_lot.o \
Python/pathconfig.o \
Python/preconfig.o \
Expand Down
3 changes: 2 additions & 1 deletion Modules/_testinternalcapi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#include "pycore_interp.h" // _PyInterpreterState_GetConfigCopy()
#include "pycore_long.h" // _PyLong_Sign()
#include "pycore_object.h" // _PyObject_IsFreed()
#include "pycore_optimizer.h" // _Py_UOpsSymType, etc.
#include "pycore_pathconfig.h" // _PyPathConfig_ClearGlobal()
#include "pycore_pyerrors.h" // _PyErr_ChainExceptions1()
#include "pycore_pystate.h" // _PyThreadState_GET()
Expand Down Expand Up @@ -1677,7 +1678,6 @@ get_py_thread_id(PyObject *self, PyObject *Py_UNUSED(ignored))
}
#endif


static PyMethodDef module_functions[] = {
{"get_configs", get_configs, METH_NOARGS},
{"get_recursion_depth", get_recursion_depth, METH_NOARGS},
Expand Down Expand Up @@ -1747,6 +1747,7 @@ static PyMethodDef module_functions[] = {
#ifdef Py_GIL_DISABLED
{"py_thread_id", get_py_thread_id, METH_NOARGS},
#endif
{"uop_symbols_test", _Py_uop_symbols_test, METH_NOARGS},
{NULL, NULL} /* sentinel */
};

Expand Down
1 change: 1 addition & 0 deletions PCbuild/_freeze_module.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@
<ClCompile Include="..\Python\object_stack.c" />
<ClCompile Include="..\Python\optimizer.c" />
<ClCompile Include="..\Python\optimizer_analysis.c" />
<ClCompile Include="..\Python\optimizer_symbols.c" />
<ClCompile Include="..\Python\parking_lot.c" />
<ClCompile Include="..\Python\pathconfig.c" />
<ClCompile Include="..\Python\perf_trampoline.c" />
Expand Down
3 changes: 3 additions & 0 deletions PCbuild/_freeze_module.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,9 @@
<ClCompile Include="..\Python\optimizer_analysis.c">
<Filter>Source Files</Filter>
</ClCompile>
<ClCompile Include="..\Python\optimizer_symbols.c">
<Filter>Python</Filter>
</ClCompile>
<ClCompile Include="..\Parser\parser.c">
<Filter>Source Files</Filter>
</ClCompile>
Expand Down
1 change: 1 addition & 0 deletions PCbuild/pythoncore.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,7 @@
<ClCompile Include="..\Python\object_stack.c" />
<ClCompile Include="..\Python\optimizer.c" />
<ClCompile Include="..\Python\optimizer_analysis.c" />
<ClCompile Include="..\Python\optimizer_symbols.c" />
<ClCompile Include="..\Python\parking_lot.c" />
<ClCompile Include="..\Python\pathconfig.c" />
<ClCompile Include="..\Python\perf_trampoline.c" />
Expand Down
3 changes: 3 additions & 0 deletions PCbuild/pythoncore.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1382,6 +1382,9 @@
<ClCompile Include="..\Python\optimizer_analysis.c">
<Filter>Python</Filter>
</ClCompile>
<ClCompile Include="..\Python\optimizer_symbols.c">
<Filter>Python</Filter>
</ClCompile>
<ClCompile Include="..\Python\parking_lot.c">
<Filter>Python</Filter>
</ClCompile>
Expand Down
Loading