gh-120323: Constant fold entire attribute loads #120344
Conversation
There was a problem hiding this comment.
The optimization looks useful, and fits in nicely with other constant propagation/removal.
A couple of general rules to follow:
Avoid side effects in functions unless that function performs only that side effect.
Keep new features and refactoring in separate PRs, where possible.
Python/optimizer_bytecodes.c
Outdated
| @@ -117,6 +117,10 @@ dummy_func(void) { | |||
|
|
|||
| op(_GUARD_TYPE_VERSION, (type_version/2, owner -- owner)) { | |||
| assert(type_version); | |||
| if (sym_is_type_subclass(owner)) { | |||
There was a problem hiding this comment.
I don't think this should be necessary.
If this case isn't handled correctly by sym_matches_type_version and sym_set_type_version below, then those need fixing.
There was a problem hiding this comment.
We need this because it might have a type version that matches, but is a type subclass, not a normal object.
There was a problem hiding this comment.
_GUARD_TYPE_VERSION guards the version number of owner's class. Whether owner is a class or not seems irrelevant.
| @@ -489,6 +486,65 @@ optimize_uops( | |||
|
|
|||
| } | |||
|
|
|||
| static inline bool | |||
| op_is_simple_load(int opcode) { | |||
There was a problem hiding this comment.
Can we avoid mixing refactorings and adding new features?
remove_unneeded_uops coould do with cleaning up, but it should be done in another PR.
| } | ||
|
|
||
| static bool | ||
| remove_simple_pops(int num_popped, _PyUOpInstruction *curr, _PyUOpInstruction *limit){ |
There was a problem hiding this comment.
If this removes simple pops, as its name suggests, why does it return a bool?
|
When you're done making the requested changes, leave the comment: |
| sym_set_is_a_class(owner); | ||
| PyTypeObject *type = _PyType_LookupByVersion(type_version); | ||
| if (type) { | ||
| if (sym_set_type_version(owner, type_version)) { |
There was a problem hiding this comment.
This records that the object's class has the version number, not that the object itself is a class and has a version number.
|
AFAICT, this PR does a number of things which would be better in their own PRs.
Can you make these separate PRs, please? |
From the PR's test case. This constant folds entire code like:
To
Using watchers.
At the moment, it's really limited and only constant folds simple class loads. In the future we can constant fold method descriptors and more.