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.

Sign up for GitHub

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

Jump to bottom

GH-111485: Increment next_instr consistently at the start of the instruction. #111486

Merged
merged 7 commits into from
Oct 31, 2023
Merged

GH-111485: Increment next_instr consistently at the start of the instruction. #111486

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
82d166b
Use instr_ptr instead of next_instr-1 in many places
markshannon Oct 27, 2023
e86ea39
Move next_instr increment to start of instruction.
markshannon Oct 27, 2023
a23102c
Don't define 'here' unless it is used
markshannon Oct 30, 2023
bdacb4e
Change a few 'frame->instr_ptr' to 'here'
markshannon Oct 30, 2023
2b28198
Change 'here' to 'this_instr' in bytecodes.c and generated cases
markshannon Oct 30, 2023
79bce6b
Merge branch 'main' into increment-next-instr-consistently
markshannon Oct 30, 2023
ad0616a
Use 'this_instr' more widely
markshannon Oct 30, 2023
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
11 changes: 6 additions & 5 deletions Include/internal/pycore_opcode_metadata.h
View file
Open in desktop

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

90 changes: 81 additions & 9 deletions Lib/test/test_generated_cases.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ def test_inst_no_args(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
spam();
DISPATCH();
}
Expand All @@ -152,6 +155,9 @@ def test_inst_one_pop(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *value;
value = stack_pointer[-1];
spam();
Expand All @@ -169,6 +175,9 @@ def test_inst_one_push(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *res;
spam();
STACK_GROW(1);
Expand All @@ -186,6 +195,9 @@ def test_inst_one_push_one_pop(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *value;
PyObject *res;
value = stack_pointer[-1];
Expand All @@ -204,6 +216,9 @@ def test_binary_op(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *right;
PyObject *left;
PyObject *res;
Expand All @@ -225,6 +240,9 @@ def test_overlap(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *right;
PyObject *left;
PyObject *result;
Expand All @@ -249,6 +267,9 @@ def test_predictions_and_eval_breaker(self):
"""
output = """
TARGET(OP1) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP1);
PREDICTED(OP1);
static_assert(INLINE_CACHE_ENTRIES_OP1 == 0, "incorrect cache size");
PyObject *arg;
Expand All @@ -259,6 +280,9 @@ def test_predictions_and_eval_breaker(self):
}

TARGET(OP3) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP3);
PyObject *arg;
PyObject *res;
arg = stack_pointer[-1];
Expand All @@ -278,6 +302,9 @@ def test_error_if_plain(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
if (cond) goto label;
DISPATCH();
}
Expand All @@ -292,6 +319,9 @@ def test_error_if_plain_with_comment(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
if (cond) goto label;
DISPATCH();
}
Expand All @@ -306,6 +336,9 @@ def test_error_if_pop(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *right;
PyObject *left;
PyObject *res;
Expand All @@ -326,12 +359,14 @@ def test_cache_effect(self):
"""
output = """
TARGET(OP) {
_Py_CODEUNIT *this_instr = frame->instr_ptr = next_instr;
next_instr += 4;
INSTRUCTION_STATS(OP);
PyObject *value;
value = stack_pointer[-1];
uint16_t counter = read_u16(&next_instr[0].cache);
uint32_t extra = read_u32(&next_instr[1].cache);
uint16_t counter = read_u16(&this_instr[1].cache);
uint32_t extra = read_u32(&this_instr[2].cache);
STACK_SHRINK(1);
next_instr += 3;
DISPATCH();
}
"""
Expand All @@ -345,6 +380,9 @@ def test_suppress_dispatch(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
goto somewhere;
}
"""
Expand All @@ -366,18 +404,24 @@ def test_macro_instruction(self):
"""
output = """
TARGET(OP1) {
_Py_CODEUNIT *this_instr = frame->instr_ptr = next_instr;
next_instr += 2;
INSTRUCTION_STATS(OP1);
PyObject *right;
PyObject *left;
right = stack_pointer[-1];
left = stack_pointer[-2];
uint16_t counter = read_u16(&next_instr[0].cache);
uint16_t counter = read_u16(&this_instr[1].cache);
op1(left, right);
next_instr += 1;
DISPATCH();
}

TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 6;
INSTRUCTION_STATS(OP);
PREDICTED(OP);
_Py_CODEUNIT *this_instr = next_instr - 6;
static_assert(INLINE_CACHE_ENTRIES_OP == 5, "incorrect cache size");
PyObject *right;
PyObject *left;
Expand All @@ -387,22 +431,24 @@ def test_macro_instruction(self):
right = stack_pointer[-1];
left = stack_pointer[-2];
{
uint16_t counter = read_u16(&next_instr[0].cache);
uint16_t counter = read_u16(&this_instr[1].cache);
op1(left, right);
}
// OP2
arg2 = stack_pointer[-3];
{
uint32_t extra = read_u32(&next_instr[3].cache);
uint32_t extra = read_u32(&this_instr[4].cache);
res = op2(arg2, left, right);
}
STACK_SHRINK(2);
stack_pointer[-1] = res;
next_instr += 5;
DISPATCH();
}

TARGET(OP3) {
frame->instr_ptr = next_instr;
next_instr += 6;
INSTRUCTION_STATS(OP3);
PyObject *right;
PyObject *left;
PyObject *arg2;
Expand All @@ -413,7 +459,6 @@ def test_macro_instruction(self):
res = op3(arg2, left, right);
STACK_SHRINK(2);
stack_pointer[-1] = res;
next_instr += 5;
DISPATCH();
}
"""
Expand All @@ -427,6 +472,9 @@ def test_array_input(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *above;
PyObject **values;
PyObject *below;
Expand All @@ -449,6 +497,9 @@ def test_array_output(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *below;
PyObject **values;
PyObject *above;
Expand All @@ -470,6 +521,9 @@ def test_array_input_output(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject **values;
PyObject *above;
values = stack_pointer - oparg;
Expand All @@ -489,6 +543,9 @@ def test_array_error_if(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject **values;
PyObject *extra;
values = stack_pointer - oparg;
Expand All @@ -509,6 +566,9 @@ def test_cond_effect(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
PyObject *cc;
PyObject *input = NULL;
PyObject *aa;
Expand Down Expand Up @@ -541,6 +601,9 @@ def test_macro_cond_effect(self):
"""
output = """
TARGET(M) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(M);
PyObject *right;
PyObject *middle;
PyObject *left;
Expand Down Expand Up @@ -580,6 +643,9 @@ def test_macro_push_push(self):
"""
output = """
TARGET(M) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(M);
PyObject *val1;
PyObject *val2;
// A
Expand Down Expand Up @@ -609,6 +675,9 @@ def test_override_inst(self):
"""
output = """
TARGET(OP) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(OP);
ham();
DISPATCH();
}
Expand All @@ -627,6 +696,9 @@ def test_override_op(self):
"""
output = """
TARGET(M) {
frame->instr_ptr = next_instr;
next_instr += 1;
INSTRUCTION_STATS(M);
ham();
DISPATCH();
}
Expand Down
18 changes: 10 additions & 8 deletions Lib/test/test_monitoring.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1378,33 +1378,35 @@ def func():
x = 4
else:
x = 6
7

self.check_events(func, recorders = JUMP_AND_BRANCH_RECORDERS, expected = [
('branch', 'func', 2, 2),
('branch', 'func', 3, 4),
('branch', 'func', 3, 6),
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now correct. See #109038 (comment)

('jump', 'func', 6, 2),
('branch', 'func', 2, 2),
('branch', 'func', 3, 3),
('branch', 'func', 3, 4),
('jump', 'func', 4, 2),
('branch', 'func', 2, 2)])
('branch', 'func', 2, 7)])

self.check_events(func, recorders = JUMP_BRANCH_AND_LINE_RECORDERS, expected = [
('line', 'get_events', 10),
('line', 'func', 1),
('line', 'func', 2),
('branch', 'func', 2, 2),
('line', 'func', 3),
('branch', 'func', 3, 4),
('branch', 'func', 3, 6),
('line', 'func', 6),
('jump', 'func', 6, 2),
('line', 'func', 2),
('branch', 'func', 2, 2),
('line', 'func', 3),
('branch', 'func', 3, 3),
('branch', 'func', 3, 4),
('line', 'func', 4),
('jump', 'func', 4, 2),
('line', 'func', 2),
('branch', 'func', 2, 2),
('branch', 'func', 2, 7),
('line', 'func', 7),
('line', 'get_events', 11)])

def test_except_star(self):
Expand Down Expand Up @@ -1434,7 +1436,7 @@ def func():
('line', 'meth', 1),
('jump', 'func', 5, 5),
('jump', 'func', 5, '[offset=114]'),
('branch', 'func', '[offset=120]', '[offset=122]'),
('branch', 'func', '[offset=120]', '[offset=124]'),
Copy link
Member Author

Choose a reason for hiding this comment

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

Likewise

('line', 'get_events', 11)])

self.check_events(func, recorders = FLOW_AND_LINE_RECORDERS, expected = [
Expand All @@ -1450,7 +1452,7 @@ def func():
('return', None),
('jump', 'func', 5, 5),
('jump', 'func', 5, '[offset=114]'),
('branch', 'func', '[offset=120]', '[offset=122]'),
('branch', 'func', '[offset=120]', '[offset=124]'),
('return', None),
('line', 'get_events', 11)])

Expand Down
Loading
Loading