From bb43ee8dd9757a791b1a137ebdaa21dd17ced731 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Tue, 20 Jun 2023 23:37:58 +0100 Subject: [PATCH 1/5] gh-105481: refactor instr flag related code into a new InstructionFlags class --- Tools/cases_generator/generate_cases.py | 118 +++++++++++++++++------- 1 file changed, 84 insertions(+), 34 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index e4ccd38bac4b49..7f956b14bb7d7b 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -234,7 +234,74 @@ def assign(self, dst: StackEffect, src: StackEffect): def cast(self, dst: StackEffect, src: StackEffect) -> str: return f"({dst.type or 'PyObject *'})" if src.type != dst.type else "" -INSTRUCTION_FLAGS = ['HAS_ARG', 'HAS_CONST', 'HAS_NAME', 'HAS_JUMP'] + +class InstructionFlags: + """Construct and manipulate instruction flags""" + + INSTRUCTION_FLAGS = ['HAS_ARG', 'HAS_CONST', 'HAS_NAME', 'HAS_JUMP'] + INSTR_FLAG_SUFFIX = "_FLAG" + + def __init__(self, has_arg: bool, has_const: bool, has_name: bool, + has_jump: bool): + + self.data = { + 'HAS_ARG': has_arg, + 'HAS_CONST': has_const, + 'HAS_NAME': has_name, + 'HAS_JUMP': has_jump, + } + assert set(self.data.keys()) == set(self.INSTRUCTION_FLAGS) + + @staticmethod + def fromInstruction(instr: "AnyInstruction"): + return InstructionFlags( + has_arg = variable_used(instr, "oparg"), + has_const = variable_used(instr, "FRAME_CO_CONSTS"), + has_name = variable_used(instr, "FRAME_CO_NAMES"), + has_jump = variable_used(instr, "JUMPBY"), + ) + + @staticmethod + def newEmpty(): + return InstructionFlags(False, False, False, False) + + def __str__(self) -> str: + flags_strs = [f"{name}{self.INSTR_FLAG_SUFFIX}" for i, name in + enumerate(self.INSTRUCTION_FLAGS) if self.is_set(name)] + if (len(flags_strs)) == 4: breakpoint() + return "0" if not flags_strs else ' | '.join(flags_strs) + + def __eq__(self, other: "InstructionFlags") -> False: + return self.bitmap() == other.bitmap() + + def __hash__(self) -> int: + return hash(self.bitmap()) + + def add(self, other: "InstructionFlags") -> None: + for name, value in other.data.items(): + self.data[name] |= value; + + def bitmap(self) -> int: + flags = 0 + for i, name in enumerate(self.INSTRUCTION_FLAGS): + flags |= (1< bool: + return self.data[name] + + @classmethod + def emit_macros(cls, formatter: Formatter): + for i, flag in enumerate(cls.INSTRUCTION_FLAGS): + flag_name = f"{flag}{cls.INSTR_FLAG_SUFFIX}" + formatter.emit(f"#define {flag_name} ({1 << i})"); + + for flag in cls.INSTRUCTION_FLAGS: + flag_name = f"{flag}{cls.INSTR_FLAG_SUFFIX}" + formatter.emit( + f"#define OPCODE_{flag}(OP) " + f"(_PyOpcode_opcode_metadata[(OP)].flags & ({flag_name}))") + @dataclasses.dataclass class Instruction: @@ -256,7 +323,7 @@ class Instruction: output_effects: list[StackEffect] unmoved_names: frozenset[str] instr_fmt: str - flags: int + instr_flags: InstructionFlags # Set later family: parser.Family | None = None @@ -285,18 +352,10 @@ def __init__(self, inst: parser.InstDef): else: break self.unmoved_names = frozenset(unmoved_names) - flag_data = { - 'HAS_ARG' : variable_used(inst, "oparg"), - 'HAS_CONST': variable_used(inst, "FRAME_CO_CONSTS"), - 'HAS_NAME' : variable_used(inst, "FRAME_CO_NAMES"), - 'HAS_JUMP' : variable_used(inst, "JUMPBY"), - } - assert set(flag_data.keys()) == set(INSTRUCTION_FLAGS) - self.flags = 0 - for i, name in enumerate(INSTRUCTION_FLAGS): - self.flags |= (1< MacroInstruction: sp = initial_sp parts: list[Component | parser.CacheEffect] = [] format = "IB" - flags = 0 + flags = InstructionFlags.newEmpty() cache = "C" for component in components: match component: @@ -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) case _: typing.assert_never(component) final_sp = sp @@ -817,7 +875,7 @@ def analyze_pseudo(self, pseudo: parser.Pseudo) -> PseudoInstruction: # Make sure the targets have the same fmt fmts = list(set([t.instr_fmt for t in targets])) assert(len(fmts) == 1) - flags_list = list(set([t.flags for t in targets])) + flags_list = list(set([t.instr_flags for t in targets])) assert(len(flags_list) == 1) return PseudoInstruction(pseudo.name, targets, fmts[0], flags_list[0]) @@ -1067,13 +1125,8 @@ def write_metadata(self) -> None: # Write type definitions self.out.emit(f"enum InstructionFormat {{ {', '.join(format_enums)} }};") - for i, flag in enumerate(INSTRUCTION_FLAGS): - self.out.emit(f"#define {flag}{INSTR_FLAG_SUFFIX} ({1 << i})"); - for flag in INSTRUCTION_FLAGS: - flag_name = f"{flag}{INSTR_FLAG_SUFFIX}" - self.out.emit( - f"#define OPCODE_{flag}(OP) " - f"(_PyOpcode_opcode_metadata[(OP)].flags & ({flag_name}))") + + InstructionFlags.emit_macros(self.out) self.out.emit("struct opcode_metadata {") with self.out.indent(): @@ -1153,25 +1206,22 @@ def write_pseudo_instrs(self) -> None: self.out.emit(f" ((OP) == {op}) || \\") self.out.emit(f" 0") - def emit_metadata_entry(self, name: str, fmt: str, flags: int) -> None: - flags_strs = [f"{name}{INSTR_FLAG_SUFFIX}" - for i, name in enumerate(INSTRUCTION_FLAGS) if (flags & (1< None: self.out.emit( - f" [{name}] = {{ true, {INSTR_FMT_PREFIX}{fmt}, {flags_s} }}," + f" [{name}] = {{ true, {INSTR_FMT_PREFIX}{fmt}, {flags} }}," ) def write_metadata_for_inst(self, instr: Instruction) -> None: """Write metadata for a single instruction.""" - self.emit_metadata_entry(instr.name, instr.instr_fmt, instr.flags) + self.emit_metadata_entry(instr.name, instr.instr_fmt, instr.instr_flags) def write_metadata_for_macro(self, mac: MacroInstruction) -> None: """Write metadata for a macro-instruction.""" - self.emit_metadata_entry(mac.name, mac.instr_fmt, mac.flags) + self.emit_metadata_entry(mac.name, mac.instr_fmt, mac.instr_flags) def write_metadata_for_pseudo(self, ps: PseudoInstruction) -> None: """Write metadata for a macro-instruction.""" - self.emit_metadata_entry(ps.name, ps.instr_fmt, ps.flags) + self.emit_metadata_entry(ps.name, ps.instr_fmt, ps.instr_flags) def write_instructions(self) -> None: """Write instructions to output file.""" From a08ac664c227730f1512b2397983dbb51b97cf21 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 21 Jun 2023 11:30:44 +0100 Subject: [PATCH 2/5] code review comments --- Tools/cases_generator/generate_cases.py | 48 ++++++++++++++----------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 7f956b14bb7d7b..c7b9c8bc544bef 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -238,38 +238,40 @@ def cast(self, dst: StackEffect, src: StackEffect) -> str: class InstructionFlags: """Construct and manipulate instruction flags""" - INSTRUCTION_FLAGS = ['HAS_ARG', 'HAS_CONST', 'HAS_NAME', 'HAS_JUMP'] + INSTRUCTION_FLAGS = ["HAS_ARG", "HAS_CONST", "HAS_NAME", "HAS_JUMP"] INSTR_FLAG_SUFFIX = "_FLAG" - def __init__(self, has_arg: bool, has_const: bool, has_name: bool, - has_jump: bool): + def __init__( + self, has_arg: bool, has_const: bool, has_name: bool, has_jump: bool + ): self.data = { - 'HAS_ARG': has_arg, - 'HAS_CONST': has_const, - 'HAS_NAME': has_name, - 'HAS_JUMP': has_jump, + "HAS_ARG": has_arg, + "HAS_CONST": has_const, + "HAS_NAME": has_name, + "HAS_JUMP": has_jump, } assert set(self.data.keys()) == set(self.INSTRUCTION_FLAGS) @staticmethod def fromInstruction(instr: "AnyInstruction"): return InstructionFlags( - has_arg = variable_used(instr, "oparg"), - has_const = variable_used(instr, "FRAME_CO_CONSTS"), - has_name = variable_used(instr, "FRAME_CO_NAMES"), - has_jump = variable_used(instr, "JUMPBY"), + has_arg=variable_used(instr, "oparg"), + has_const=variable_used(instr, "FRAME_CO_CONSTS"), + has_name=variable_used(instr, "FRAME_CO_NAMES"), + has_jump=variable_used(instr, "JUMPBY"), ) @staticmethod def newEmpty(): return InstructionFlags(False, False, False, False) - def __str__(self) -> str: - flags_strs = [f"{name}{self.INSTR_FLAG_SUFFIX}" for i, name in - enumerate(self.INSTRUCTION_FLAGS) if self.is_set(name)] - if (len(flags_strs)) == 4: breakpoint() - return "0" if not flags_strs else ' | '.join(flags_strs) + def names(self) -> list[str]: + return [ + f"{name}{self.INSTR_FLAG_SUFFIX}" + for name in self.INSTRUCTION_FLAGS + if self.is_set(name) + ] def __eq__(self, other: "InstructionFlags") -> False: return self.bitmap() == other.bitmap() @@ -291,14 +293,14 @@ def is_set(self, name: str) -> bool: return self.data[name] @classmethod - def emit_macros(cls, formatter: Formatter): + def emit_macros(cls, out: Formatter): for i, flag in enumerate(cls.INSTRUCTION_FLAGS): flag_name = f"{flag}{cls.INSTR_FLAG_SUFFIX}" - formatter.emit(f"#define {flag_name} ({1 << i})"); + out.emit(f"#define {flag_name} ({1 << i})"); for flag in cls.INSTRUCTION_FLAGS: flag_name = f"{flag}{cls.INSTR_FLAG_SUFFIX}" - formatter.emit( + out.emit( f"#define OPCODE_{flag}(OP) " f"(_PyOpcode_opcode_metadata[(OP)].flags & ({flag_name}))") @@ -1206,9 +1208,13 @@ def write_pseudo_instrs(self) -> None: self.out.emit(f" ((OP) == {op}) || \\") self.out.emit(f" 0") - def emit_metadata_entry(self, name: str, fmt: str, flags: InstructionFlags) -> None: + def emit_metadata_entry( + self, name: str, fmt: str, flags: InstructionFlags + ) -> None: + if not (names := flags.names()): + names.append("0") self.out.emit( - f" [{name}] = {{ true, {INSTR_FMT_PREFIX}{fmt}, {flags} }}," + f" [{name}] = {{ true, {INSTR_FMT_PREFIX}{fmt}, {' | '.join(names)} }}," ) def write_metadata_for_inst(self, instr: Instruction) -> None: From 8fcd65f568d6c49918c7e1d5edd8cc33199221ce Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 21 Jun 2023 16:53:12 +0100 Subject: [PATCH 3/5] make InstructionFlags a dataclass --- Tools/cases_generator/generate_cases.py | 77 +++++++++++-------------- 1 file changed, 35 insertions(+), 42 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index c7b9c8bc544bef..4dfd0bdc61bf03 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -234,45 +234,33 @@ def assign(self, dst: StackEffect, src: StackEffect): def cast(self, dst: StackEffect, src: StackEffect) -> str: return f"({dst.type or 'PyObject *'})" if src.type != dst.type else "" - +@dataclasses.dataclass class InstructionFlags: """Construct and manipulate instruction flags""" - INSTRUCTION_FLAGS = ["HAS_ARG", "HAS_CONST", "HAS_NAME", "HAS_JUMP"] - INSTR_FLAG_SUFFIX = "_FLAG" - - def __init__( - self, has_arg: bool, has_const: bool, has_name: bool, has_jump: bool - ): + HAS_ARG_FLAG: bool + HAS_CONST_FLAG: bool + HAS_NAME_FLAG: bool + HAS_JUMP_FLAG: bool - self.data = { - "HAS_ARG": has_arg, - "HAS_CONST": has_const, - "HAS_NAME": has_name, - "HAS_JUMP": has_jump, + def __post_init__(self): + self.bitmask = { + name : (1 << i) for i, name in enumerate(self.names()) } - assert set(self.data.keys()) == set(self.INSTRUCTION_FLAGS) @staticmethod def fromInstruction(instr: "AnyInstruction"): return InstructionFlags( - has_arg=variable_used(instr, "oparg"), - has_const=variable_used(instr, "FRAME_CO_CONSTS"), - has_name=variable_used(instr, "FRAME_CO_NAMES"), - has_jump=variable_used(instr, "JUMPBY"), + HAS_ARG_FLAG=variable_used(instr, "oparg"), + HAS_CONST_FLAG=variable_used(instr, "FRAME_CO_CONSTS"), + HAS_NAME_FLAG=variable_used(instr, "FRAME_CO_NAMES"), + HAS_JUMP_FLAG=variable_used(instr, "JUMPBY"), ) @staticmethod def newEmpty(): return InstructionFlags(False, False, False, False) - def names(self) -> list[str]: - return [ - f"{name}{self.INSTR_FLAG_SUFFIX}" - for name in self.INSTRUCTION_FLAGS - if self.is_set(name) - ] - def __eq__(self, other: "InstructionFlags") -> False: return self.bitmap() == other.bitmap() @@ -280,29 +268,32 @@ def __hash__(self) -> int: return hash(self.bitmap()) def add(self, other: "InstructionFlags") -> None: - for name, value in other.data.items(): - self.data[name] |= value; + for name, value in dataclasses.asdict(other).items(): + if value: + setattr(self, name, value) + + def names(self, value=None): + if value is None: + return dataclasses.asdict(self).keys() + return [n for n, v in dataclasses.asdict(self).items() if v == value] def bitmap(self) -> int: flags = 0 - for i, name in enumerate(self.INSTRUCTION_FLAGS): - flags |= (1< bool: - return self.data[name] - @classmethod def emit_macros(cls, out: Formatter): - for i, flag in enumerate(cls.INSTRUCTION_FLAGS): - flag_name = f"{flag}{cls.INSTR_FLAG_SUFFIX}" - out.emit(f"#define {flag_name} ({1 << i})"); + flags = cls.newEmpty() + for name, value in flags.bitmask.items(): + out.emit(f"#define {name} ({value})"); - for flag in cls.INSTRUCTION_FLAGS: - flag_name = f"{flag}{cls.INSTR_FLAG_SUFFIX}" + for name, value in flags.bitmask.items(): out.emit( - f"#define OPCODE_{flag}(OP) " - f"(_PyOpcode_opcode_metadata[(OP)].flags & ({flag_name}))") + f"#define OPCODE_{name[:-len('_FLAG')]}(OP) " + f"(_PyOpcode_opcode_metadata[(OP)].flags & ({name}))") @dataclasses.dataclass @@ -357,7 +348,7 @@ def __init__(self, inst: parser.InstDef): self.instr_flags = InstructionFlags.fromInstruction(inst) - if self.instr_flags.is_set('HAS_ARG'): + if self.instr_flags.HAS_ARG_FLAG: fmt = "IB" else: fmt = "IX" @@ -1211,10 +1202,12 @@ def write_pseudo_instrs(self) -> None: def emit_metadata_entry( self, name: str, fmt: str, flags: InstructionFlags ) -> None: - if not (names := flags.names()): - names.append("0") + flag_names = flags.names(value=True) + if not flag_names: + flag_names.append("0") self.out.emit( - f" [{name}] = {{ true, {INSTR_FMT_PREFIX}{fmt}, {' | '.join(names)} }}," + f" [{name}] = {{ true, {INSTR_FMT_PREFIX}{fmt}," + f" {' | '.join(flag_names)} }}," ) def write_metadata_for_inst(self, instr: Instruction) -> None: From d383eaa246c340de63e2e90ca50e3603a83ddabb Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 21 Jun 2023 22:53:26 +0100 Subject: [PATCH 4/5] remove __hash__ and __eq__ --- Tools/cases_generator/generate_cases.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index 4dfd0bdc61bf03..efd4fd701478c0 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -261,12 +261,6 @@ def fromInstruction(instr: "AnyInstruction"): def newEmpty(): return InstructionFlags(False, False, False, False) - def __eq__(self, other: "InstructionFlags") -> False: - return self.bitmap() == other.bitmap() - - def __hash__(self) -> int: - return hash(self.bitmap()) - def add(self, other: "InstructionFlags") -> None: for name, value in dataclasses.asdict(other).items(): if value: From f34174df787a0ce43ba1cf8979fec42c924cb044 Mon Sep 17 00:00:00 2001 From: Irit Katriel Date: Wed, 21 Jun 2023 23:48:20 +0100 Subject: [PATCH 5/5] hash the bitmap instead of the InstructionFlags object --- Tools/cases_generator/generate_cases.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/Tools/cases_generator/generate_cases.py b/Tools/cases_generator/generate_cases.py index efd4fd701478c0..1afdeef41f0efc 100644 --- a/Tools/cases_generator/generate_cases.py +++ b/Tools/cases_generator/generate_cases.py @@ -862,9 +862,8 @@ def analyze_pseudo(self, pseudo: parser.Pseudo) -> PseudoInstruction: # Make sure the targets have the same fmt fmts = list(set([t.instr_fmt for t in targets])) assert(len(fmts) == 1) - flags_list = list(set([t.instr_flags for t in targets])) - assert(len(flags_list) == 1) - return PseudoInstruction(pseudo.name, targets, fmts[0], flags_list[0]) + assert(len(list(set([t.instr_flags.bitmap() for t in targets]))) == 1) + return PseudoInstruction(pseudo.name, targets, fmts[0], targets[0].instr_flags) def analyze_instruction( self, instr: Instruction, stack: list[StackEffect], sp: int