Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
iritkatriel merged 5 commits intopython:mainfromiritkatriel:flags
Jun 21, 2023

Conversation

iritkatriel
Copy link
Member

@iritkatrieliritkatriel commentedJun 20, 2023•
edited by bedevere-bot
Loading

  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

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

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

Choose a reason for hiding this comment

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

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

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

gvanrossum reacted with thumbs up emoji
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 intogh-105924.

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.

LGTM!

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

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

@iritkatrieliritkatrielenabled auto-merge (squash)June 21, 2023 22:51
@iritkatrieliritkatriel merged commitc01da28 intopython:mainJun 21, 2023
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull requestJun 22, 2023
bentasker pushed a commit to bentasker/cpython that referenced this pull requestJun 23, 2023
@iritkatrieliritkatriel deleted the flags branchJuly 25, 2023 18:02
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@iritkatriel@gvanrossum@bedevere-bot
[8]ページ先頭
©2009-2025 Movatter.jp