Skip to content
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-106581: Add 10 new opcodes by allowing assert(kwnames == NULL) #106707

Merged
merged 5 commits into from
Jul 17, 2023

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Jul 13, 2023

Not all of these are useful, but I think they all work (I tried the same thing last week, when it crashed mysteriously; I've fixed so many crashes since then that I think it'll work now).

This leaves 14 17 CALL specializations that need to be split (or need gh-106603, allowing oparg and one cache entry per uop).

Some of the opcodes here surely need to be split into separate guards and actions; that can be done as needed.

@gvanrossum
Copy link
Member Author

gvanrossum commented Jul 14, 2023

So the test failure, in particular test.test_descr.ClassPropertiesAndMethods.test_keywords, shows that there's an unresolved problem. And of course it has to do with the kwnames variable. There's an instance of this in the Tier 1 interpreter and another in the Tier 2 interpreter. But what happens if a Tier 2 instruction that uses kwnames deoptimizes? The corresponding Tier 1 instruction will find kwnames == NULL since there is no room in the API to pass the kwnames back from Tier 2 to Tier 1.

Looks like this will have to wait until gh-105848, where it is proposed that the kwnames gets pushed on the stack instead of being a local variable in the eval loop.

A fair number of these instructions don't really use kwnames, they just contain an assert(kwnames == NULL) statement. I think I'll play around with a way to allow this assertion but not other uses of kwnames.

@gvanrossum
Copy link
Member Author

Most test failures are expected (for a run with -Xuops on by default). But the test_list failure pointed to a recent bug in the Tier 2 interpreter (not interpreted here). Fixing in gh-106756.

@gvanrossum gvanrossum changed the title gh-106581: Allow using 'kwnames' -- this adds 14 new opcodes gh-106581: Allow using 'kwnames' -- this adds 10 new opcodes Jul 14, 2023
Not all of these are useful, but I think they all work.
It leaves another 14 CALL specializations that need to be split
(or need pythongh-106603, allowing oparg *and* one cache entry per uop).
But allow `assert(kwnames == NULL)`.
@gvanrossum
Copy link
Member Author

gvanrossum commented Jul 15, 2023

PS. I have a benchmark running on this with -Xuops enabled by default. That should give us an idea of how much slower the Tier 2 interpreter is.

@gvanrossum
Copy link
Member Author

The benchmark (using uops wherever we can) comes in 4% slower than main. That's good to know.

@gvanrossum gvanrossum marked this pull request as ready for review July 15, 2023 23:53
@gvanrossum
Copy link
Member Author

@markshannon What do you think of this?

@gvanrossum gvanrossum changed the title gh-106581: Allow using 'kwnames' -- this adds 10 new opcodes gh-106581: Add 10 new opcodes by allowing assert(kwnames == NULL) Jul 16, 2023
@gvanrossum gvanrossum merged commit 2b94a05 into python:main Jul 17, 2023
@gvanrossum gvanrossum deleted the simple-calls branch July 17, 2023 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants