Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-104909: Split some more insts into ops#109943
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
gvanrossum commented Sep 27, 2023 • edited by bedevere-app bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by bedevere-app bot
Uh oh!
There was an error while loading. Please reload this page.
7bdddb0 to b54eee3Comparegvanrossum commented Sep 27, 2023
Another couple of questions for @brandtbucher (or @markshannon).
|
Fidget-Spinner commented Sep 27, 2023
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? |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
gvanrossum commented Sep 27, 2023
We already have other actions using
That's still a bit in the future though. And I'm not sure how often |
gvanrossum commented Sep 27, 2023
Going to merge this soon, basically as-is. Perf is same as base (1.00 faster). |
gvanrossum commented Sep 27, 2023
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? |
gvanrossum commented Sep 27, 2023
Well, okay, some tests fail consistently, so I have to bisect. Sorry for the accusations. |
gvanrossum commented Sep 27, 2023
Looks like |
I had dropped one of the DEOPT_IF() calls! :-(
bedevere-bot commented Sep 27, 2023
|
bedevere-bot commented Sep 27, 2023
|
bedevere-bot commented Sep 27, 2023
|
bedevere-bot commented Sep 28, 2023
|
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! :-(
vstinner commented Sep 28, 2023
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 -vbefore 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?