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-111485: Allow arbitrary annotations on instructions and micro-ops. #111697

Merged

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Nov 3, 2023

This allows us to be explicit about classifications of uops that are currently encoded in the code generator or analysis phase.

As an example, I've added a "specializing" annotation to denote which micro-ops specialize, rather than perform an action.
Using that we can avoid emitting the specialization code in tier 2.

A possible extension is to allow more than one annotation, but this PR only allows one for now.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Nice work, but I think the Meta folks are going to be upset that you are breaking "override". They use that feature. @itamaro @carljm

Also, test_generated_cases is now broken. One possible fix would be to add "annotation" to the set annotations in lexer.py.

@gvanrossum
Copy link
Member

Maybe we need to add a test for override to test_generated_cases.py, so we know it doesn't rot.

@jbower-fb
Copy link
Contributor

Nice work, but I think the Meta folks are going to be upset that you are breaking "override". They use that feature. @itamaro @carljm

Yes, we definitely still want this please.

Maybe we need to add a test for override to test_generated_cases.py, so we know it doesn't rot.

There were some tests, but this PR removed them.

@markshannon is there some reason override can't co-exist with annotations?

@markshannon
Copy link
Member Author

Yes, we definitely still want this please.

I'll add back the functionality. I assumed it was a feature we added for development and hadn't cleaned up.
What is it for?

@markshannon markshannon force-pushed the allow-annotations-for-bytecodes branch from f4b5df7 to 140a681 Compare November 6, 2023 14:59
@markshannon
Copy link
Member Author

Since it is not clear to me if and when override is used, I've change the code to allow multiple annotations per op/inst.

@jbower-fb
Copy link
Contributor

Great, thank you for doing that.

The motivation for override is to make it a bit easier/safer for things like Cinder where we need to make our own tweaks to the interpreter. See #102021 where this was introduced.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Awesome!

Tools/cases_generator/parsing.py Outdated Show resolved Hide resolved
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
@markshannon markshannon merged commit 931f443 into python:main Nov 7, 2023
32 checks passed
hugovk pushed a commit to hugovk/cpython that referenced this pull request Nov 8, 2023
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@markshannon markshannon deleted the allow-annotations-for-bytecodes branch August 6, 2024 10:17
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
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.

3 participants