-
-
Notifications
You must be signed in to change notification settings - Fork 31.3k
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.
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
gh-104909: Split some more insts into ops #109943
Conversation
7bdddb0
to
b54eee3
Compare
Another couple of questions for @brandtbucher (or @markshannon).
|
Not Mark or Brandt, but on the optimization side, we would have to treat this as an impure op, which makes most of the optimization opportunities go away. However, we're likely converting some of these to LOAD_CONST anyways so that might mitigate it? |
We already have other actions using
That's still a bit in the future though. And I'm not sure how often |
Going to merge this soon, basically as-is. Perf is same as base (1.00 faster). |
I can't find a persistent pattern in the failing tests. Maybe it's a timeout in multiprocessing? I believe @vstinner recently made flaky tests more likely to cause CI failure, maybe it's that? |
Well, okay, some tests fail consistently, so I have to bisect. Sorry for the accusations. |
Looks like |
I had dropped one of the DEOPT_IF() calls! :-(
|
|
|
|
These are the most popular specializations of `LOAD_ATTR` and `STORE_ATTR` that weren't already viable uops: * Split LOAD_ATTR_METHOD_WITH_VALUES * Split LOAD_ATTR_METHOD_NO_DICT * Split LOAD_ATTR_SLOT * Split STORE_ATTR_SLOT * Split STORE_ATTR_INSTANCE_VALUE Also: * Add `-v` flag to code generator which prints a list of non-viable uops (easter-egg: it can print execution counts -- see source) * Double _Py_UOP_MAX_TRACE_LENGTH to 128 I had dropped one of the DEOPT_IF() calls! :-(
Right. Tests are now run with |
These are the most popular specializations of `LOAD_ATTR` and `STORE_ATTR` that weren't already viable uops: * Split LOAD_ATTR_METHOD_WITH_VALUES * Split LOAD_ATTR_METHOD_NO_DICT * Split LOAD_ATTR_SLOT * Split STORE_ATTR_SLOT * Split STORE_ATTR_INSTANCE_VALUE Also: * Add `-v` flag to code generator which prints a list of non-viable uops (easter-egg: it can print execution counts -- see source) * Double _Py_UOP_MAX_TRACE_LENGTH to 128 I had dropped one of the DEOPT_IF() calls! :-(
I started by adding some code that shows the most deserving non-viable opcodes. Also doubling max trace size again.
I could do this all day, but the stats show there are diminishing returns. Output from
python3 Tools/cases_generator/generate_cases.py -v
before splitting the top 5:I took the counts from here (follow the pystats raw link).
LOAD_ATTR_GETATTRIBUTE_OVERRIDDEN
? It was the same on the weekly run a week before; the week before that (Sept 10) the file format was different. @brandtbucher any intuition on this?