Skip to content

GH-125837: Rework the instructions for loading constants and returning values. #125878

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

Closed
markshannon wants to merge 12 commits into from
Closed

GH-125837: Rework the instructions for loading constants and returning values. #125878

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
2a61cfc
Replace RETURN_VLAUE/RETURN_CONST with RETURN_VALUE_FUNC/RETURN_VALUE…
markshannon Oct 22, 2024
42a526c
Add LOAD_CONST_IMMORTAL
markshannon Oct 23, 2024
2a3b1e2
Fix up all the tests apart from test_dis
markshannon Oct 23, 2024
031f320
Update optimizer
markshannon Oct 23, 2024
cb5252b
Fix up test_dis
markshannon Oct 23, 2024
892edbc
Fix up a couple of comments
markshannon Oct 23, 2024
5ce99ca
Apply suggestions from code review
markshannon Oct 24, 2024
325c5fe
Clarify comment
markshannon Oct 24, 2024
6149153
Remove unused function
markshannon Oct 24, 2024
59a554a
Update test
markshannon Oct 24, 2024
bcb4e65
Update dis docs
markshannon Oct 24, 2024
f3383e5
Remove link to remove instruction
markshannon Oct 24, 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
Prev Previous commit
Next Next commit
Fix up all the tests apart from test_dis
  • Loading branch information
@markshannon
markshannon committed Oct 23, 2024
commit 2a3b1e2c5ca126ea342dcbe81c477f202455972a
2 changes: 1 addition & 1 deletion Lib/test/test_ast/test_ast.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2303,7 +2303,7 @@ def get_load_const(self, tree):
co = compile(tree, '<string>', 'exec')
consts = []
for instr in dis.get_instructions(co):
if instr.opname in ('LOAD_CONST_MORTAL', 'LOAD_CONST_IMMORTAL'):
if instr.opname in ('LOAD_CONST', 'LOAD_CONST_IMMORTAL'):
consts.append(instr.argval)
return consts

Expand Down
7 changes: 4 additions & 3 deletions Lib/test/test_code.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,10 +256,11 @@ def func():
return x
code = func.__code__

# different co_name, co_varnames, co_consts
# Different co_name, co_varnames, co_consts.
# Must have the same number of constants and
# variables or we get crashes.
Comment thread
iritkatriel marked this conversation as resolved.
def func2():
y = 2
z = 3
return y
code2 = func2.__code__

Expand Down Expand Up @@ -408,7 +409,7 @@ def test_co_positions_artificial_instructions(self):
],
[
("PUSH_EXC_INFO", None),
("LOAD_CONST_IMMORTAL", None), # artificial 'None'
("LOAD_CONST", None), # artificial 'None'
("STORE_NAME", "e"), # XX: we know the location for this
("DELETE_NAME", "e"),
("RERAISE", 1),
Expand Down
26 changes: 13 additions & 13 deletions Lib/test/test_compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1045,7 +1045,7 @@ def unused_block_while_else():

for func in funcs:
opcodes = list(dis.get_instructions(func))
self.assertLessEqual(len(opcodes), 3)
self.assertLessEqual(len(opcodes), 4)
self.assertEqual('RETURN_VALUE_FUNC', opcodes[-1].opname)
self.assertEqual(None, opcodes[-1].argval)

Expand All @@ -1063,8 +1063,8 @@ def continue_in_while():
# Check that we did not raise but we also don't generate bytecode
for func in funcs:
opcodes = list(dis.get_instructions(func))
self.assertEqual(2, len(opcodes))
self.assertEqual('RETURN_VALUE_FUNC', opcodes[1].opname)
self.assertEqual(3, len(opcodes))
self.assertEqual('RETURN_VALUE_FUNC', opcodes[-1].opname)
self.assertEqual(None, opcodes[1].argval)

def test_consts_in_conditionals(self):
Expand All @@ -1088,7 +1088,7 @@ def or_false(x):
opcodes = list(dis.get_instructions(func))
self.assertLessEqual(len(opcodes), 3)
self.assertIn('LOAD_', opcodes[-2].opname)
self.assertEqual('RETURN_VALUE', opcodes[-1].opname)
self.assertEqual('RETURN_VALUE_FUNC', opcodes[-1].opname)

def test_imported_load_method(self):
sources = [
Expand Down Expand Up @@ -1760,7 +1760,7 @@ def test_multiline_generator_expression(self):
line=1, end_line=2, column=1, end_column=8, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=1, end_line=2, column=1, end_column=8, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_FUNC',
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_GEN',
line=4, end_line=4, column=7, end_column=14, occurrence=1)

def test_multiline_async_generator_expression(self):
Expand All @@ -1777,7 +1777,7 @@ def test_multiline_async_generator_expression(self):
self.assertIsInstance(compiled_code, types.CodeType)
self.assertOpcodeSourcePositionIs(compiled_code, 'YIELD_VALUE',
line=1, end_line=2, column=1, end_column=8, occurrence=2)
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_FUNC',
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_GEN',
line=1, end_line=6, column=0, end_column=32, occurrence=1)

def test_multiline_list_comprehension(self):
Expand Down Expand Up @@ -1815,7 +1815,7 @@ async def f():
line=2, end_line=3, column=5, end_column=12, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=2, end_line=3, column=5, end_column=12, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_FUNC',
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_GEN',
line=2, end_line=7, column=4, end_column=36, occurrence=1)

def test_multiline_set_comprehension(self):
Expand Down Expand Up @@ -1891,7 +1891,7 @@ async def f():
line=2, end_line=3, column=5, end_column=11, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'JUMP_BACKWARD',
line=2, end_line=3, column=5, end_column=11, occurrence=1)
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_FUNC',
self.assertOpcodeSourcePositionIs(compiled_code, 'RETURN_VALUE_GEN',
line=2, end_line=7, column=4, end_column=36, occurrence=1)

def test_matchcase_sequence(self):
Expand Down Expand Up @@ -2204,17 +2204,17 @@ def f():
start_line, end_line, _, _ = instr.positions
self.assertEqual(start_line, end_line)

# Expect three load None instructions for the no-exception __exit__ call,
# and one RETURN_VALUE.
# Expect three `LOAD_CONST None` instructions for the no-exception __exit__ call,
# and one for the RETURN_VALUE.
Comment thread
markshannon marked this conversation as resolved.
Outdated
# They should all have the locations of the context manager ('xyz').

load_none = [instr for instr in dis.get_instructions(f) if
instr.opname == 'LOAD_CONST' and instr.argval is None]
return_value = [instr for instr in dis.get_instructions(f) if
instr.opname == 'RETURN_VALUE']
instr.opname == 'RETURN_VALUE_FUNC']

self.assertEqual(len(load_none), 3)
self.assertEqual(len(return_value), 1)
self.assertEqual(len(load_none), 4)
self.assertEqual(len(return_value), 2)
for instr in load_none + return_value:
start_line, end_line, start_col, end_col = instr.positions
self.assertEqual(start_line, f.__code__.co_firstlineno + 1)
Expand Down
4 changes: 2 additions & 2 deletions Lib/test/test_compiler_assemble.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ def test_exception_table(self):
# code for "try: pass\n except: pass"
insts = [
('RESUME', 0),
('SETUP_FINALLY', 3),
('SETUP_FINALLY', 4),
('LOAD_CONST', 0),
('RETURN_VALUE_FUNC', None),
('SETUP_CLEANUP', 8),
('SETUP_CLEANUP', 10),
('PUSH_EXC_INFO', None),
('POP_TOP', None),
('POP_EXCEPT', None),
Expand Down
10 changes: 5 additions & 5 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,11 +222,11 @@ def bug42562():

# Extended arg followed by NOP
code_bug_45757 = bytes([
opcode.opmap['EXTENDED_ARG'], 0x01, # EXTENDED_ARG 0x01
opcode.opmap['NOP'], 0xFF, # NOP 0xFF
opcode.opmap['EXTENDED_ARG'], 0x01, # EXTENDED_ARG 0x01
opcode.opmap['LOAD_CONST'], 0x29, # LOAD_CONST 0x29
opcode.opmap['RETURN_VALUE'], 0x00, # RETURN_VALUE 0x00
opcode.opmap['EXTENDED_ARG'], 0x01,
opcode.opmap['NOP'], 0xFF,
opcode.opmap['EXTENDED_ARG'], 0x01,
opcode.opmap['LOAD_CONST'], 0x29,
opcode.opmap['RETURN_VALUE_FUNC'], 0x00,
])

dis_bug_45757 = """\
Expand Down
3 changes: 3 additions & 0 deletions Lib/test/test_embed.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,9 @@ def is_specialized(f):
opname in opcode._specialized_opmap
# Exclude superinstructions:
and "__" not in opname
# LOAD_CONST_IMMORTAL is "specialized", but is
# inserted during quickening.
and opname != "LOAD_CONST_IMMORTAL"
):
return True
return False
Expand Down
5 changes: 5 additions & 0 deletions Lib/test/test_monitoring.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1205,6 +1205,7 @@ def func1():
('instruction', 'func1', 10),
('instruction', 'func1', 12),
('instruction', 'func1', 14),
('instruction', 'func1', 16),
('line', 'get_events', 11)])

def test_c_call(self):
Expand All @@ -1229,6 +1230,7 @@ def func2():
('instruction', 'func2', 40),
('instruction', 'func2', 42),
('instruction', 'func2', 44),
('instruction', 'func2', 46),
('line', 'get_events', 11)])

def test_try_except(self):
Expand Down Expand Up @@ -1262,6 +1264,7 @@ def func3():
('instruction', 'func3', 30),
('instruction', 'func3', 32),
('instruction', 'func3', 34),
('instruction', 'func3', 36),
('line', 'get_events', 11)])

def test_with_restart(self):
Expand All @@ -1282,6 +1285,7 @@ def func1():
('instruction', 'func1', 10),
('instruction', 'func1', 12),
('instruction', 'func1', 14),
('instruction', 'func1', 16),
('line', 'get_events', 11)])

sys.monitoring.restart_events()
Expand All @@ -1298,6 +1302,7 @@ def func1():
('instruction', 'func1', 10),
('instruction', 'func1', 12),
('instruction', 'func1', 14),
('instruction', 'func1', 16),
('line', 'get_events', 11)])

def test_turn_off_only_instruction(self):
Expand Down
43 changes: 24 additions & 19 deletions Lib/test/test_peepholer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def test_folding_of_tuples_of_constants(self):
# One LOAD_CONST for the tuple, one for the None return value
load_consts = [instr for instr in dis.get_instructions(code)
if instr.opname == 'LOAD_CONST']
self.assertEqual(len(load_consts), 1)
self.assertEqual(len(load_consts), 2)
self.check_lnotab(code)

# Bug 1053819: Tuple of constants misidentified when presented with:
Expand Down Expand Up @@ -337,7 +337,7 @@ def f(x):
return x
self.assertNotInBytecode(f, 'LOAD_CONST', None)
returns = [instr for instr in dis.get_instructions(f)
if instr.opname == 'RETURN_VALUE']
if instr.opname == 'RETURN_VALUE_FUNC']
self.assertEqual(len(returns), 1)
self.check_lnotab(f)

Expand All @@ -351,7 +351,7 @@ def f(cond, true_value, false_value):
self.assertNotInBytecode(f, 'JUMP_FORWARD')
self.assertNotInBytecode(f, 'JUMP_BACKWARD')
returns = [instr for instr in dis.get_instructions(f)
if instr.opname == 'RETURN_VALUE']
if instr.opname == 'RETURN_VALUE_FUNC']
self.assertEqual(len(returns), 2)
self.check_lnotab(f)

Expand Down Expand Up @@ -981,19 +981,19 @@ def test_conditional_jump_forward_non_const_condition(self):
('LOAD_NAME', 1, 11),
('POP_JUMP_IF_TRUE', lbl := self.Label(), 12),
('LOAD_CONST', 2, 13),
('RETURN_VALUE', None, 13),
('RETURN_VALUE_FUNC', None, 13),
lbl,
('LOAD_CONST', 3, 14),
('RETURN_VALUE', None, 14),
('RETURN_VALUE_FUNC', None, 14),
]
expected_insts = [
('LOAD_NAME', 1, 11),
('POP_JUMP_IF_TRUE', lbl := self.Label(), 12),
('LOAD_CONST', 2, 13),
('RETURN_VALUE', None, 13),
('LOAD_CONST', 1, 13),
('RETURN_VALUE_FUNC', None, 13),
lbl,
('LOAD_CONST', 2, 13),
('RETURN_VALUE', None, 13),
('LOAD_CONST', 2, 14),
('RETURN_VALUE_FUNC', None, 14),
]
self.cfg_optimization_test(insts,
expected_insts,
Expand All @@ -1010,13 +1010,13 @@ def test_conditional_jump_forward_const_condition(self):
('LOAD_CONST', 2, 13),
lbl,
('LOAD_CONST', 3, 14),
('RETURN_VALUE', None, 14),
('RETURN_VALUE_FUNC', None, 14),
]
expected_insts = [
('NOP', None, 11),
('NOP', None, 12),
('LOAD_CONST', 3, 14),
('RETURN_VALUE', None, 14),
('LOAD_CONST', 1, 14),
('RETURN_VALUE_FUNC', None, 14),
]
self.cfg_optimization_test(insts,
expected_insts,
Expand Down Expand Up @@ -1061,16 +1061,18 @@ def test_except_handler_label(self):
('SETUP_FINALLY', handler := self.Label(), 10),
('POP_BLOCK', None, -1),
('LOAD_CONST', 1, 11),
('RETURN_VALUE', None, None),
('RETURN_VALUE_FUNC', None, 11),
handler,
('LOAD_CONST', 2, 12),
('RETURN_VALUE', None, None),
('RETURN_VALUE_FUNC', None, 12),
]
expected_insts = [
('SETUP_FINALLY', handler := self.Label(), 10),
('RETURN_CONST', 1, 11),
('LOAD_CONST', 1, 11),
('RETURN_VALUE_FUNC', None, 11),
handler,
('RETURN_CONST', 2, 12),
('LOAD_CONST', 2, 12),
('RETURN_VALUE_FUNC', None, 12),
]
self.cfg_optimization_test(insts, expected_insts, consts=list(range(5)))

Expand All @@ -1093,7 +1095,8 @@ def test_no_unsafe_static_swap(self):
('NOP', None, 3),
('STORE_FAST', 1, 4),
('POP_TOP', None, 4),
('RETURN_CONST', 0)
('LOAD_CONST', 0, 5),
('RETURN_VALUE', None, 5)
]
self.cfg_optimization_test(insts, expected_insts, consts=list(range(3)), nlocals=1)

Expand All @@ -1114,7 +1117,8 @@ def test_dead_store_elimination_in_same_lineno(self):
('NOP', None, 3),
('POP_TOP', None, 4),
('STORE_FAST', 1, 4),
('RETURN_CONST', 0, 5)
('LOAD_CONST', 0, 5),
('RETURN_VALUE', None, 5)
]
self.cfg_optimization_test(insts, expected_insts, consts=list(range(3)), nlocals=1)

Expand All @@ -1136,7 +1140,8 @@ def test_no_dead_store_elimination_in_different_lineno(self):
('STORE_FAST', 1, 4),
('STORE_FAST', 1, 5),
('STORE_FAST', 1, 6),
('RETURN_CONST', 0, 5)
('LOAD_CONST', 0, 5),
('RETURN_VALUE', None, 5)
]
self.cfg_optimization_test(insts, expected_insts, consts=list(range(3)), nlocals=1)

Expand Down