Skip to content

Conversation

@markshannon
Copy link
Member

@markshannonmarkshannon commented Dec 19, 2023

This PR moves the table generators to the new architecture.

Copy link
Member

@gvanrossumgvanrossum 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; ^~~~~~~~~~~~~~~~~~ 

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
MemberAuthor

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.

Copy link
Member

@iritkatrieliritkatriel left a comment

Choose a reason for hiding this comment

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

LGTM.

@markshannonmarkshannon merged commit e96f260 into python:mainDec 20, 2023
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
@markshannonmarkshannon 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@markshannon@iritkatriel@gvanrossum@brandtbucher