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-118423: Add INSTRUCTION_SIZE macro to code generator #125467

Merged
merged 34 commits into from
Oct 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
ec8c701
Add INSTRUCTION_SIZE macro
tomasr8 Oct 14, 2024
09c2f59
Use INSTRUCTION_SIZE in bytecodes.c
tomasr8 Oct 14, 2024
287eb92
Add tests
tomasr8 Oct 14, 2024
c76fce4
Update docs
tomasr8 Oct 14, 2024
e3f3b07
Add news entry
tomasr8 Oct 14, 2024
ed5b375
Fix mypy error
tomasr8 Oct 14, 2024
870341d
Treat INSTRUCTION_SIZE as an identifier
tomasr8 Oct 14, 2024
421ebda
Regenerate executor cases
tomasr8 Oct 14, 2024
5154f7f
Re-add newline
tomasr8 Oct 14, 2024
e7e83dc
Use replaceText()
tomasr8 Oct 14, 2024
101a5fd
Emit size directly
tomasr8 Oct 15, 2024
1338628
Merge branch 'main' into instr-size
tomasr8 Oct 15, 2024
ee461d5
Pass instruction in tier2
tomasr8 Oct 16, 2024
7135103
Fix INSTRUCTION_SIZE in tier2
tomasr8 Oct 16, 2024
715ce31
Make mypy happy
tomasr8 Oct 16, 2024
8235321
Fix tests
tomasr8 Oct 16, 2024
148070e
Remove unused imports
tomasr8 Oct 17, 2024
3e6b592
Ensure all instructions have the same size
tomasr8 Oct 17, 2024
6e78ec0
Add tests
tomasr8 Oct 17, 2024
75e6f6c
Regen files
tomasr8 Oct 17, 2024
a3c0d10
Make mypy happy
tomasr8 Oct 17, 2024
7235d2d
Fix tests
tomasr8 Oct 20, 2024
5cfb423
Raise instead of assert when size is None
tomasr8 Oct 21, 2024
2190784
Add an `instruction_size` field to the Uop class
tomasr8 Oct 21, 2024
33343dd
Remove extra white space
tomasr8 Oct 21, 2024
f9ecb16
Raise analysis_error instead of ValueError
tomasr8 Oct 21, 2024
abfaaff
Fix wording
tomasr8 Oct 21, 2024
3f3ad10
Make mypy happy
tomasr8 Oct 21, 2024
a8f72ef
Simplify test
tomasr8 Oct 28, 2024
8138a06
Replace assert with raise
tomasr8 Oct 28, 2024
772f803
Simplify code
tomasr8 Oct 28, 2024
a718cfb
Remove some code repetition
tomasr8 Oct 28, 2024
a10d4ee
Simplify INSTRUCTION_SIZE check
tomasr8 Oct 29, 2024
3ae7a5d
Simplify code
tomasr8 Oct 29, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions Lib/test/test_generated_cases.py
Original file line number Diff line number Diff line change
Expand Up @@ -1371,6 +1371,37 @@ def test_stack_save_only(self):
with self.assertRaises(SyntaxError):
self.run_cases_test(input, output)

def test_instruction_size_macro(self):
input = """
inst(OP, (--)) {
frame->return_offset = INSTRUCTION_SIZE;
}
"""

output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
frame->return_offset = 1 ;
DISPATCH();
}
"""
self.run_cases_test(input, output)

Copy link
Member

Choose a reason for hiding this comment

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

Add a test for another instruction that is a different length, but uses OP:

macro(OP2) = unused/1 + OP;

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

Copy link
Member

Choose a reason for hiding this comment

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

That test should fail, as the INSTRUCTION_SIZE is inconsistent.
The tier 2 INSTRUCTION_SIZE cannot be both 1 and 2.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, sorry I mistakenly thought it should only fail for tier 2 hence I was only checking the instruction in the tier 2 generator. I extended the check to tier 1 as well and the test now fails as expected.

# Two instructions of different sizes referencing the same
# uop containing the `INSTRUCTION_SIZE` macro is not allowed.
input = """
inst(OP, (--)) {
frame->return_offset = INSTRUCTION_SIZE;
}
macro(OP2) = unused/1 + OP;
"""

output = "" # No output needed as this should raise an error.
with self.assertRaisesRegex(SyntaxError, "All instructions containing a uop"):
self.run_cases_test(input, output)


class TestGeneratedAbstractCases(unittest.TestCase):
def setUp(self) -> None:
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Add a new ``INSTRUCTION_SIZE`` macro to the cases generator which returns
the current instruction size.
22 changes: 11 additions & 11 deletions Python/bytecodes.c
Original file line number Diff line number Diff line change
Expand Up @@ -843,7 +843,7 @@ dummy_func(
new_frame->localsplus[0] = container;
new_frame->localsplus[1] = sub;
INPUTS_DEAD();
frame->return_offset = (uint16_t)(1 + INLINE_CACHE_ENTRIES_BINARY_SUBSCR);
frame->return_offset = INSTRUCTION_SIZE;
}

macro(BINARY_SUBSCR_GETITEM) =
Expand Down Expand Up @@ -1103,8 +1103,8 @@ dummy_func(
gen->gi_frame_state = FRAME_EXECUTING;
gen->gi_exc_state.previous_item = tstate->exc_info;
tstate->exc_info = &gen->gi_exc_state;
assert(next_instr - this_instr + oparg <= UINT16_MAX);
frame->return_offset = (uint16_t)(next_instr - this_instr + oparg);
assert(INSTRUCTION_SIZE + oparg <= UINT16_MAX);
frame->return_offset = (uint16_t)(INSTRUCTION_SIZE + oparg);
assert(gen_frame->previous == NULL);
gen_frame->previous = frame;
DISPATCH_INLINED(gen_frame);
Expand Down Expand Up @@ -1149,8 +1149,8 @@ dummy_func(
gen->gi_frame_state = FRAME_EXECUTING;
gen->gi_exc_state.previous_item = tstate->exc_info;
tstate->exc_info = &gen->gi_exc_state;
assert(1 + INLINE_CACHE_ENTRIES_SEND + oparg <= UINT16_MAX);
frame->return_offset = (uint16_t)(1 + INLINE_CACHE_ENTRIES_SEND + oparg);
assert(INSTRUCTION_SIZE + oparg <= UINT16_MAX);
frame->return_offset = (uint16_t)(INSTRUCTION_SIZE + oparg);
gen_frame->previous = frame;
}

Expand Down Expand Up @@ -2257,7 +2257,7 @@ dummy_func(
new_frame->localsplus[0] = owner;
DEAD(owner);
new_frame->localsplus[1] = PyStackRef_FromPyObjectNew(name);
frame->return_offset = (uint16_t)(next_instr - this_instr);
frame->return_offset = INSTRUCTION_SIZE;
DISPATCH_INLINED(new_frame);
}

Expand Down Expand Up @@ -3055,7 +3055,7 @@ dummy_func(
tstate->exc_info = &gen->gi_exc_state;
gen_frame->previous = frame;
// oparg is the return offset from the next instruction.
frame->return_offset = (uint16_t)(1 + INLINE_CACHE_ENTRIES_FOR_ITER + oparg);
frame->return_offset = (uint16_t)(INSTRUCTION_SIZE + oparg);
}

macro(FOR_ITER_GEN) =
Expand Down Expand Up @@ -3334,7 +3334,7 @@ dummy_func(
if (new_frame == NULL) {
ERROR_NO_POP();
}
frame->return_offset = (uint16_t)(next_instr - this_instr);
frame->return_offset = INSTRUCTION_SIZE;
DISPATCH_INLINED(new_frame);
}
/* Callable is not a normal Python function */
Expand Down Expand Up @@ -4196,8 +4196,8 @@ dummy_func(
if (new_frame == NULL) {
ERROR_NO_POP();
}
assert(next_instr - this_instr == 1 + INLINE_CACHE_ENTRIES_CALL_KW);
frame->return_offset = 1 + INLINE_CACHE_ENTRIES_CALL_KW;
assert(INSTRUCTION_SIZE == 1 + INLINE_CACHE_ENTRIES_CALL_KW);
frame->return_offset = INSTRUCTION_SIZE;
DISPATCH_INLINED(new_frame);
}
/* Callable is not a normal Python function */
Expand Down Expand Up @@ -4463,7 +4463,7 @@ dummy_func(
if (new_frame == NULL) {
ERROR_NO_POP();
}
assert(next_instr - this_instr == 1);
assert(INSTRUCTION_SIZE == 1);
frame->return_offset = 1;
DISPATCH_INLINED(new_frame);
}
Expand Down
10 changes: 5 additions & 5 deletions Python/executor_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 12 additions & 12 deletions Python/generated_cases.c.h

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

33 changes: 33 additions & 0 deletions Tools/cases_generator/analyzer.py
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,8 @@ class Uop:
implicitly_created: bool = False
replicated = 0
replicates: "Uop | None" = None
# Size of the instruction(s), only set for uops containing the INSTRUCTION_SIZE macro
instruction_size: int | None = None

def dump(self, indent: str) -> None:
print(
Expand Down Expand Up @@ -1079,6 +1081,35 @@ def add_instruction(name: str) -> None:
return instmap, len(no_arg), min_instrumented


def get_instruction_size_for_uop(instructions: dict[str, Instruction], uop: Uop) -> int | None:
"""Return the size of the instruction that contains the given uop or
`None` if the uop does not contains the `INSTRUCTION_SIZE` macro.

If there is more than one instruction that contains the uop,
ensure that they all have the same size.
"""
for tkn in uop.body:
if tkn.text == "INSTRUCTION_SIZE":
break
else:
return None

size = None
for inst in instructions.values():
if uop in inst.parts:
if size is None:
size = inst.size
if size != inst.size:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if size != inst.size:
elif size != inst.size:

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed this to an if because otherwise mypy complains that the elif is unreachable:

Tools/cases_generator/analyzer.py:1103: error: Statement is unreachable  [unreachable]

Copy link
Member

Choose a reason for hiding this comment

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

OK, leave it then.

raise analysis_error(
"All instructions containing a uop with the `INSTRUCTION_SIZE` macro "
f"must have the same size: {size} != {inst.size}",
tkn
)
if size is None:
raise analysis_error(f"No instruction containing the uop '{uop.name}' was found", tkn)
return size


def analyze_forest(forest: list[parser.AstNode]) -> Analysis:
instructions: dict[str, Instruction] = {}
uops: dict[str, Uop] = {}
Expand Down Expand Up @@ -1122,6 +1153,8 @@ def analyze_forest(forest: list[parser.AstNode]) -> Analysis:
continue
if target.text in instructions:
instructions[target.text].is_target = True
for uop in uops.values():
uop.instruction_size = get_instruction_size_for_uop(instructions, uop)
# Special case BINARY_OP_INPLACE_ADD_UNICODE
# BINARY_OP_INPLACE_ADD_UNICODE is not a normal family member,
# as it is the wrong size, but we need it to maintain an
Expand Down
20 changes: 17 additions & 3 deletions Tools/cases_generator/generators_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@
analysis_error,
)
from cwriter import CWriter
from typing import Callable, Mapping, TextIO, Iterator, Iterable
from typing import Callable, TextIO, Iterator, Iterable
from lexer import Token
from stack import Stack, Local, Storage, StackError
from stack import Storage, StackError

# Set this to true for voluminous output showing state of stack and locals
PRINT_STACKS = False
Expand Down Expand Up @@ -118,7 +118,8 @@ def __init__(self, out: CWriter):
"PyStackRef_CLOSE": self.stackref_close,
"PyStackRef_CLOSE_SPECIALIZED": self.stackref_close,
"PyStackRef_AsPyObjectSteal": self.stackref_steal,
"DISPATCH": self.dispatch
"DISPATCH": self.dispatch,
"INSTRUCTION_SIZE": self.instruction_size,
}
self.out = out

Expand Down Expand Up @@ -357,6 +358,19 @@ def reload_stack(
self.emit_reload(storage)
return True

def instruction_size(self,
tkn: Token,
tkn_iter: TokenIterator,
uop: Uop,
storage: Storage,
inst: Instruction | None,
) -> bool:
"""Replace the INSTRUCTION_SIZE macro with the size of the current instruction."""
if uop.instruction_size is None:
raise analysis_error("The INSTRUCTION_SIZE macro requires uop.instruction_size to be set", tkn)
self.out.emit(f" {uop.instruction_size} ")
return True

def _print_storage(self, storage: Storage) -> None:
if PRINT_STACKS:
self.out.start_line()
Expand Down
6 changes: 4 additions & 2 deletions Tools/cases_generator/interpreter_definition.md
Original file line number Diff line number Diff line change
Expand Up @@ -178,15 +178,17 @@ list of annotations and their meanings are as follows:

### Special functions/macros

The C code may include special functions that are understood by the tools as
The C code may include special functions and macros that are understood by the tools as
part of the DSL.

Those functions include:
Those include:

* `DEOPT_IF(cond, instruction)`. Deoptimize if `cond` is met.
* `ERROR_IF(cond, label)`. Jump to error handler at `label` if `cond` is true.
* `DECREF_INPUTS()`. Generate `Py_DECREF()` calls for the input stack effects.
* `SYNC_SP()`. Synchronizes the physical stack pointer with the stack effects.
* `INSTRUCTION_SIZE`. Replaced with the size of the instruction which is equal
to `1 + INLINE_CACHE_ENTRIES`.

Note that the use of `DECREF_INPUTS()` is optional -- manual calls
to `Py_DECREF()` or other approaches are also acceptable
Expand Down
1 change: 0 additions & 1 deletion Tools/cases_generator/tier1_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
write_header,
type_and_null,
Emitter,
TokenIterator,
)
from cwriter import CWriter
from typing import TextIO
Expand Down
Loading