8000 gh-120323: Remove class check for redundant _CHECK_ATTR_CLASS by Fidget-Spinner · Pull Request #121002 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-120323: Remove class check for redundant _CHECK_ATTR_CLASS #121002

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

gh-120323: Remove class check for redundant _CHECK_ATTR_CLASS #121002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7fa937c
Constant fold entire class loads
Fidget-Spinner Jun 11, 2024
85a5f36
Update test_opt.py
Fidget-Spinner Jun 11, 2024
8c0d50c
only remove second type check
Fidget-Spinner Jun 11, 2024
011477f
fix refcounting issue
Fidget-Spinner Jun 11, 2024
7763a32
Address reviews partially
Fidget-Spinner Jun 15, 2024
3330a62
Merge remote-tracking branch 'upstream/main' into optimizer_constant_…
Fidget-Spinner Jun 15, 2024
b9f6059
Remove redundant type check _CHECK_ATTR_CLASS
Fidget-Spinner Jun 25, 2024
2af786c
reduce diff
Fidget-Spinner Jun 25, 2024
de210f0
reduce diff
Fidget-Spinner Jun 25, 2024
ea904cf
remove stray edits
Fidget-Spinner Jun 25, 2024
aff925f
remove refactoring
Fidget-Spinner Jun 25, 2024
b316ae4
Merge remote-tracking branch 'upstream/main' into optimizer_remove_cl…
Fidget-Spinner Jun 26, 2024
b693d68
Cleanup and address review applying Mark's changes
Fidget-Spinner Jun 26, 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
2 changes: 2 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 @@ -238,6 +238,7 @@ typedef struct _Py_UOpsContext _Py_UOpsContext;
extern bool _Py_uop_sym_is_null(_Py_UopsSymbol *sym);
extern bool _Py_uop_sym_is_not_null(_Py_UopsSymbol *sym);
extern bool _Py_uop_sym_is_const(_Py_UopsSymbol *sym);
extern bool _Py_uop_sym_is_a_class(_Py_UopsSymbol *sym, unsigned int version);
extern PyObject *_Py_uop_sym_get_const(_Py_UopsSymbol *sym);
extern _Py_UopsSymbol *_Py_uop_sym_new_unknown(_Py_UOpsContext *ctx);
extern _Py_UopsSymbol *_Py_uop_sym_new_not_null(_Py_UOpsContext *ctx);
Expand All @@ -253,6 +254,7 @@ extern void _Py_uop_sym_set_non_null(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym);
extern void _Py_uop_sym_set_type(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, PyTypeObject *typ);
extern bool _Py_uop_sym_set_type_version(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, unsigned int version);
extern void _Py_uop_sym_set_const(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, PyObject *const_val);
extern void _Py_uop_sym_set_is_a_class(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, unsigned int version);
extern bool _Py_uop_sym_is_bottom(_Py_UopsSymbol *sym);
extern int _Py_uop_sym_truthiness(_Py_UopsSymbol *sym);
extern PyTypeObject *_Py_uop_sym_get_type(_Py_UopsSymbol *sym);
Expand Down
22 changes: 22 additions & 0 deletions Lib/test/test_capi/test_opt.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1481,5 +1481,27 @@ def fn(a):
fn(A())


def test_type_attribute_type_check_eliminated(self):
ns = {}
src = textwrap.dedent("""
class MyEnum:
A = 1

def testfunc(n):
for i in range(n):
enum = MyEnum
# Note: can't just put MyEnum here because we lose
# information between constant promotions.
x = enum.A
y = enum.A
""")
exec(src, ns, ns)
testfunc = ns['testfunc']
_, ex = self._run_with_optimizer(testfunc, TIER2_THRESHOLD * 2)
self.assertIsNotNone(ex)
uops = get_opnames(ex)
self.assertLessEqual(uops.count("_CHECK_ATTR_CLASS"), 1)


if __name__ == "__main__":
unittest.main()
11 changes: 11 additions & 0 deletions Objects/typeobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,6 +947,17 @@ PyType_Watch(int watcher_id, PyObject* obj)
return 0;
}

int
PyType_IsWatched(int watcher_id, PyObject* obj)
{
if (!PyType_Check(obj)) {
PyErr_SetString(PyExc_ValueError, "Cannot check non-type");
return -1;
}
PyTypeObject *type = (PyTypeObject *)obj;
return type->tp_watched & (1 << watcher_id);
}

int
PyType_Unwatch(int watcher_id, PyObject* obj)
{
Expand Down
2 changes: 2 additions & 0 deletions Python/optimizer_analysis.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,6 +321,7 @@ remove_globals(_PyInterpreterFrame *frame, _PyUOpInstruction *buffer,
#define sym_new_not_null _Py_uop_sym_new_not_null
#define sym_new_type _Py_uop_sym_new_type
#define sym_is_null _Py_uop_sym_is_null
#define sym_is_a_class _Py_uop_sym_is_a_class
#define sym_new_const _Py_uop_sym_new_const
#define sym_new_null _Py_uop_sym_new_null
#define sym_has_type _Py_uop_sym_has_type
Expand All @@ -329,6 +330,7 @@ remove_globals(_PyInterpreterFrame *frame, _PyUOpInstruction *buffer,
#define sym_matches_type_version _Py_uop_sym_matches_type_version
#define sym_set_null(SYM) _Py_uop_sym_set_null(ctx, SYM)
#define sym_set_non_null(SYM) _Py_uop_sym_set_non_null(ctx, SYM)
#define sym_set_is_a_class(SYM, VERSION) _Py_uop_sym_set_is_a_class(ctx, SYM, VERSION)
#define sym_set_type(SYM, TYPE) _Py_uop_sym_set_type(ctx, SYM, TYPE)
#define sym_set_type_version(SYM, VERSION) _Py_uop_sym_set_type_version(ctx, SYM, VERSION)
#define sym_set_const(SYM, CNST) _Py_uop_sym_set_const(ctx, SYM, CNST)
Expand Down
22 changes: 22 additions & 0 deletions Python/optimizer_bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,12 @@ dummy_func(void) {

op(_GUARD_TYPE_VERSION, (type_version/2, owner -- owner)) {
assert(type_version);
// This is a contradiction.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessarily a contradiction. Py_TYPE(owner)->tp_version == type_version can be true even if owner is a class.

I suspect we don't check the version number of metaclasses anywhere now, but it is definitely possible for enums.

sym_matches_type_version should check for the contradiction, so this check shouldn't be necessary.

if (sym_is_a_class(owner, type_version)) {
ctx->done = true;
ctx->contradiction = true;
break;
}
if (sym_matches_type_version(owner, type_version)) {
REPLACE_OP(this_instr, _NOP, 0, 0);
} else {
Expand All @@ -138,6 +144,22 @@ dummy_func(void) {
}
}

op(_CHECK_ATTR_CLASS, (type_version/2, owner -- owner)) {
assert(type_version);
if (sym_is_a_class(owner, type_version) &&
sym_matches_type_version(owner, type_version)) {
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect. sym_matches_type_version matches the type version of the symbol's class, not that of the symbol.

Copy link
Member

Choose a reason for hiding this comment

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

Please stop marking comments as resolved when they are not.
It wastes a lot of time forcing me double check all resolved issues, if I cannot trust that they are resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry again, this was an old PR and was back when I more freely resolved things.

REPLACE_OP(this_instr, _NOP, 0, 0);
} else {
PyTypeObject *type = _PyType_LookupByVersion(type_version);
if (type) {
sym_set_is_a_class(owner, type_version);
PyType_Watch(TYPE_WATCHER_ID, (PyObject *)type);
_Py_BloomFilter_Add(dependencies, type);
}

}
}

op(_GUARD_BOTH_FLOAT, (left, right -- left, right)) {
if (sym_matches_type(left, &PyFloat_Type)) {
if (sym_matches_type(right, &PyFloat_Type)) {
Expand Down
21 changes: 21 additions & 0 deletions Python/optimizer_cases.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 21 additions & 3 deletions Python/optimizer_symbols.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,12 @@
*/

// Flags for below.
#define IS_NULL 1 << 0
#define NOT_NULL 1 << 1
#define NO_SPACE 1 << 2
#define IS_NULL 1 << 0
#define NOT_NULL 1 << 1
#define NO_SPACE 1 << 2
#define IS_TYPE_SUBCLASS 1 << 3
#define HAS_TYPE_VERSION_GUARD 1 << 4
#define HAS_CLASS_VERSION_GUARD 1 << 5

#ifdef Py_DEBUG
static inline int get_lltrace(void) {
Expand Down Expand Up @@ -121,6 +124,12 @@ _Py_uop_sym_is_null(_Py_UopsSymbol *sym)
return sym->flags == IS_NULL;
}

bool
_Py_uop_sym_is_a_class(_Py_UopsSymbol *sym, unsigned int type_version)
Copy link
Member

Choose a reason for hiding this comment

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

These need tests.
Specifically tests setting the symbol to be a class, and then setting its class's version.

{
return (sym->flags & IS_TYPE_SUBCLASS) && (sym->type_version == type_version);
}

bool
_Py_uop_sym_is_const(_Py_UopsSymbol *sym)
{
Expand Down Expand Up @@ -207,6 +216,15 @@ _Py_uop_sym_set_non_null(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym)
sym_set_flag(sym, NOT_NULL);
}

void
_Py_uop_sym_set_is_a_class(_Py_UOpsContext *ctx, _Py_UopsSymbol *sym, unsigned int type_version)
Copy link
Member

Choose a reason for hiding this comment

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

This needs a better name. The is in the middle reads strangely.
Maybe just _Py_uop_sym_set_class?

{
if (sym->typ != NULL && !PyType_Check(sym->typ)) {
sym_set_bottom(ctx, sym);
}
sym_set_flag(sym, IS_TYPE_SUBCLASS);
_Py_uop_sym_set_type_version(ctx, sym, type_version);
}

_Py_UopsSymbol *
_Py_uop_sym_new_unknown(_Py_UOpsContext *ctx)
Expand Down
Loading
0