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

@iritkatrieliritkatriel commented Jun 20, 2023

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.

@gvanrossumgvanrossum 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.

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

Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
format += cache
cache = "0"
flags |= instr.flags
flags.add(instr.instr_flags)

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.

At this point, in my optimizer work, I have a need to prevent one flag ("IS_UOP") from being propagated. How would you do that using the new API?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

We can add an arg to this function to tell it which flags to skip?

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.

Yeah, something like exclude: set[str] | None = None would work.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Let's add it when we need it. I don't like adding code that is neither used nor tested.

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.

I can add it after I merge your code into gh-105924.

Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated
Comment threadTools/cases_generator/generate_cases.py Outdated

@gvanrossumgvanrossum 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.

LGTM!

@iritkatriel

Copy link
Copy Markdown
MemberAuthor

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

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.

@gvanrossumgvanrossum 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.

That works too!

@iritkatriel

Copy link
Copy Markdown
MemberAuthor

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:mainJun 21, 2023
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Jun 22, 2023
bentasker pushed a commit to bentasker/cpython that referenced this pull request Jun 23, 2023
@iritkatriel
iritkatriel deleted the flags branch July 25, 2023 18:02
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.

3 participants

@iritkatriel@gvanrossum@bedevere-bot