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-105481: refactor instr flag related code into a new InstructionFlags class #105950

Merged
merged 5 commits into from
Jun 21, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatriel iritkatriel commented Jun 20, 2023

  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.

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.

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

Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
@@ -803,7 +861,7 @@ def analyze_macro(self, macro: parser.Macro) -> MacroInstruction:
for _ in range(ce.size):
format += cache
cache = "0"
flags |= instr.flags
flags.add(instr.instr_flags)
Copy link
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
Member Author

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
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
Member Author

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

Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
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.

LGTM!

@iritkatriel
Copy link
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
Copy link
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.

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.

That works too!

@iritkatriel
Copy link
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
14 checks passed
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 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