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: Generate instruction and uop metadata #113287

Merged
merged 55 commits into from
Dec 20, 2023

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Dec 19, 2023

This PR moves the table generators to the new architecture.

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.

A big step forward! After this it's time to kill dead code. (Maybe use coverage.py to find what's unused, it works pretty well with this codebase.)

When I try to build after ./configure --with-pydebug --enable-pystats I get an error; that file is missing #include "pycore_uop_metadata.h":

Python/specialize.c:247:21: error: use of undeclared identifier '_PyOpcode_uop_name'; did you mean '_PyOpcode_OpName'?
            names = _PyOpcode_uop_name;
                    ^~~~~~~~~~~~~~~~~~

Makefile.pre.in Outdated Show resolved Hide resolved
Python/bytecodes.c Outdated Show resolved Hide resolved
Python/optimizer.c Outdated Show resolved Hide resolved
out.emit(f"#define {uop.name} {next_id}\n")
next_id += 1

out.emit(f"#define MAX_UOP_ID {next_id-1}\n")
Copy link
Member

Choose a reason for hiding this comment

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

I realize with a name like this we sort of need this to be the last valid one rather than the first one beyond, but in the general 0-based scheme of things this feels jarring (and you have to remember to add 1 to compute the table size in optimizer.c). So may I please for a name and value that suggest/use next_id instead of next_id-1? E.g. UOP_ID_LIMIT.

Copy link
Member Author

Choose a reason for hiding this comment

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

The meaning of MAX_UOP_ID is clear, But the meaning of UOP_ID_LIMIT is not (at least not to me).
The -1 and +1 is a bit clunky, but I think it is preferable to loosing clarity.
We could change the name to MAX_UOP_ID_PLUS_ONE, but that is just clunky in a different way.

Tools/cases_generator/stack.py Outdated Show resolved Hide resolved
Tools/cases_generator/py_metadata_generator.py Outdated Show resolved Hide resolved
Tools/cases_generator/generators_common.py Outdated Show resolved Hide resolved
Tools/cases_generator/py_metadata_generator.py Outdated Show resolved Hide resolved
Tools/cases_generator/uop_metadata_generator.py Outdated Show resolved Hide resolved
Tools/cases_generator/uop_metadata_generator.py Outdated Show resolved Hide resolved
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

LGTM.

@markshannon markshannon merged commit e96f260 into python:main Dec 20, 2023
36 checks passed
ryan-duve pushed a commit to ryan-duve/cpython that referenced this pull request Dec 26, 2023
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@markshannon markshannon deleted the generate-uop-metadata branch August 6, 2024 10:18
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.

4 participants