◐ Shell
reader mode source ↗
Skip to content

gh-105481: refactor instr flag related code into a new InstructionFlags class#105950

Merged
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:flags
Jun 21, 2023
Merged

gh-105481: refactor instr flag related code into a new InstructionFlags class#105950
iritkatriel merged 5 commits into
python:mainfrom
iritkatriel:flags

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Jun 20, 2023 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  1. save the complete flags data on the instruction (not just the bitmap)
  2. refactor all the code into a new flag management class.

This will help later when we want to use the flags for things like generating hasarg, etc.

@bedevere-bot bedevere-bot mentioned this pull request Jun 20, 2023
Closed
@iritkatriel iritkatriel requested a review from gvanrossum June 20, 2023 22:42
gvanrossum
gvanrossum reviewed Jun 20, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

I like refactoring this, but I'd like to go a little beyond just packaging up the old code in a class.

gvanrossum
gvanrossum approved these changes Jun 21, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM!

@iritkatriel

iritkatriel commented Jun 21, 2023

Copy link
Copy Markdown
Member Author

It's doing this now that I removed the hash and eq:

Traceback (most recent call last):
  File "/Users/iritkatriel/src/cpython-1/./Tools/cases_generator/generate_cases.py", line 1422, in <module>
    main()
  File "/Users/iritkatriel/src/cpython-1/./Tools/cases_generator/generate_cases.py", line 1414, in main
    a.analyze()  # Prints messages and sets a.errors on failure
    ^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/./Tools/cases_generator/generate_cases.py", line 708, in analyze
    self.analyze_macros_and_pseudos()
  File "/Users/iritkatriel/src/cpython-1/./Tools/cases_generator/generate_cases.py", line 827, in analyze_macros_and_pseudos
    self.pseudo_instrs[name] = self.analyze_pseudo(pseudo)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/iritkatriel/src/cpython-1/./Tools/cases_generator/generate_cases.py", line 865, in analyze_pseudo
    flags_list = list(set([t.instr_flags for t in targets]))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: unhashable type: 'InstructionFlags'

@gvanrossum

gvanrossum commented Jun 21, 2023

Copy link
Copy Markdown
Member

It's doing this now that I removed the hash and eq:

Traceback (most recent call last):
[...]
  File "/Users/iritkatriel/src/cpython-1/./Tools/cases_generator/generate_cases.py", line 865, in analyze_pseudo
    flags_list = list(set([t.instr_flags for t in targets]))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: unhashable type: 'InstructionFlags'

Oh dang. Dataclasses don't generate a hash by default. You can either put the __hash__ back or write @dataclass(unsafe_hash=True). The "unsafe" part concerns me.

gvanrossum
gvanrossum approved these changes Jun 21, 2023
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

That works too!

@iritkatriel

iritkatriel commented Jun 21, 2023

Copy link
Copy Markdown
Member Author

I changed it to not hash the data class object but the bitmap representation. It's just for an assertion that all the flags of a pseudo-op's targets are the same.

@iritkatriel iritkatriel enabled auto-merge (squash) June 21, 2023 22:51
@iritkatriel iritkatriel merged commit c01da28 into python:main Jun 21, 2023
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Jun 22, 2023
@iritkatriel @gvanrossum
pythongh-105481: refactor instr flag related code into a new Instruct… …
6d532c7 
…ionFlags class (python#105950)
bentasker pushed a commit to bentasker/cpython that referenced this pull request Jun 23, 2023
@iritkatriel @bentasker
pythongh-105481: refactor instr flag related code into a new Instruct… …
deaaa08 
…ionFlags class (python#105950)
@iritkatriel iritkatriel deleted the flags branch July 25, 2023 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@iritkatriel @gvanrossum @bedevere-bot