Skip to content

gh-105775: Convert LOAD_CLOSURE to a pseudo-op #106059

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

Merged
gvanrossum merged 14 commits into from
Jun 29, 2023
Merged

gh-105775: Convert LOAD_CLOSURE to a pseudo-op #106059

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
21d086e
gh-105775: Demote LOAD_CLOSURE to a pseudo-op
polynomialherder Jun 24, 2023
6921930
gh-106044: Fix whitespace
polynomialherder Jun 24, 2023
b111d10
gh-105775: Regenerate generated_cases.c.h
polynomialherder Jun 25, 2023
bbdbb99
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 25, 2023
93b3138
gh-105775: Add unit test, correct NEWS/docs
polynomialherder Jun 27, 2023
32c1a70
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 27, 2023
4103b9a
Merge branch 'Remove-LOAD-CLOSURE-105775' of github.com:polynomialher…
polynomialherder Jun 27, 2023
269933e
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 27, 2023
a069593
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 27, 2023
988189d
gh-105775: Update documentation, whitespace tweaks
polynomialherder Jun 27, 2023
968601d
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 27, 2023
46fa9d5
Update news item
polynomialherder Jun 28, 2023
2b677b6
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 28, 2023
ab8ce45
Merge branch 'main' into Remove-LOAD-CLOSURE-105775
polynomialherder Jun 28, 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
24 changes: 12 additions & 12 deletions Doc/library/dis.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1291,18 +1291,6 @@ iterations of the loop.
.. versionadded:: 3.11


.. opcode:: LOAD_CLOSURE (i)

Pushes a reference to the cell contained in slot ``i`` of the "fast locals"
storage. The name of the variable is ``co_fastlocalnames[i]``.

Note that ``LOAD_CLOSURE`` is effectively an alias for ``LOAD_FAST``.
It exists to keep bytecode a little more readable.

.. versionchanged:: 3.11
``i`` is no longer offset by the length of ``co_varnames``.


.. opcode:: LOAD_DEREF (i)

Loads the cell contained in slot ``i`` of the "fast locals" storage.
Expand Down Expand Up @@ -1725,6 +1713,18 @@ but are replaced by real opcodes or removed before bytecode is generated.
Undirected relative jump instructions which are replaced by their
directed (forward/backward) counterparts by the assembler.

.. opcode:: LOAD_CLOSURE (i)

Pushes a reference to the cell contained in slot ``i`` of the "fast locals"
storage.

Note that ``LOAD_CLOSURE`` is replaced with ``LOAD_FAST`` in the assembler.
It exists to keep bytecode a little more readable.
Copy link
Copy Markdown
Member

@carljm carljm Jun 26, 2023

Choose a reason for hiding this comment

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

I don't think this sentence is accurate anymore (if it ever was). LOAD_CLOSURE still exists in order to keep fix_cell_offsets working. Pseudo-instructions can't make bytecode more readable, since they don't exist in bytecode.

I would just remove this sentence.


.. versionchanged:: 3.13
This opcode was demoted from full opcode to pseudo-instruction
Copy link
Copy Markdown
Member

@iritkatriel iritkatriel Jun 27, 2023

Choose a reason for hiding this comment

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

Suggested change
This opcode was demoted from full opcode to pseudo-instruction
This opcode is now a pseudo-instruction.



.. opcode:: LOAD_METHOD

Optimized unbound method lookup. Emitted as a ``LOAD_ATTR`` opcode
Expand Down
15 changes: 8 additions & 7 deletions Include/internal/pycore_opcode.h
View file
Open in desktop

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

22 changes: 11 additions & 11 deletions Include/opcode.h
View file
Open in desktop

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

3 changes: 2 additions & 1 deletion Lib/importlib/_bootstrap_external.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,7 @@ def _write_atomic(path, data, mode=0o666):
# Python 3.13a1 3553 (Add SET_FUNCTION_ATTRIBUTE)
# Python 3.13a1 3554 (more efficient bytecodes for f-strings)
# Python 3.13a1 3555 (generate specialized opcodes metadata from bytecodes.c)
# Python 3.13a1 3556 (Remove LOAD_CLOSURE)

# Python 3.14 will start with 3600

Expand All @@ -467,7 +468,7 @@ def _write_atomic(path, data, mode=0o666):
# Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array
# in PC/launcher.c must also be updated.

MAGIC_NUMBER = (3555).to_bytes(2, 'little') + b'\r\n'
MAGIC_NUMBER = (3556).to_bytes(2, 'little') + b'\r\n'

_RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c

Expand Down
3 changes: 1 addition & 2 deletions Lib/opcode.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,8 +198,6 @@ def pseudo_op(name, op, real_ops):
jrel_op('JUMP_BACKWARD_NO_INTERRUPT', 134) # Number of words to skip (backwards)
def_op('MAKE_CELL', 135)
hasfree.append(135)
def_op('LOAD_CLOSURE', 136)
hasfree.append(136)
def_op('LOAD_DEREF', 137)
hasfree.append(137)
def_op('STORE_DEREF', 138)
Expand Down Expand Up @@ -291,6 +289,7 @@ def pseudo_op(name, op, real_ops):
pseudo_op('LOAD_SUPER_METHOD', 263, ['LOAD_SUPER_ATTR'])
pseudo_op('LOAD_ZERO_SUPER_METHOD', 264, ['LOAD_SUPER_ATTR'])
pseudo_op('LOAD_ZERO_SUPER_ATTR', 265, ['LOAD_SUPER_ATTR'])
pseudo_op('LOAD_CLOSURE', 267, ['LOAD_FAST'])

pseudo_op('STORE_FAST_MAYBE_NULL', 266, ['STORE_FAST'])

Expand Down
16 changes: 8 additions & 8 deletions Lib/test/test_dis.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,7 +685,7 @@ def foo(x):

%3d RESUME 0

%3d LOAD_CLOSURE 0 (y)
%3d LOAD_FAST 0 (y)
BUILD_TUPLE 1
LOAD_CONST 1 (<code object foo at 0x..., file "%s", line %d>)
MAKE_FUNCTION
Expand All @@ -709,7 +709,7 @@ def foo(x):
%3d RESUME 0

%3d LOAD_GLOBAL 1 (NULL + list)
LOAD_CLOSURE 0 (x)
LOAD_FAST 0 (x)
BUILD_TUPLE 1
LOAD_CONST 1 (<code object <genexpr> at 0x..., file "%s", line %d>)
MAKE_FUNCTION
Expand Down Expand Up @@ -1596,8 +1596,8 @@ def _prepare_test_cases():
Instruction(opname='MAKE_CELL', opcode=135, arg=1, argval='b', argrepr='b', offset=2, start_offset=2, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='RESUME', opcode=151, arg=0, argval=0, argrepr='', offset=4, start_offset=4, starts_line=1, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CONST', opcode=100, arg=5, argval=(3, 4), argrepr='(3, 4)', offset=6, start_offset=6, starts_line=2, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CLOSURE', opcode=136, arg=0, argval='a', argrepr='a', offset=8, start_offset=8, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CLOSURE', opcode=136, arg=1, argval='b', argrepr='b', offset=10, start_offset=10, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_FAST', opcode=124, arg=0, argval='a', argrepr='a', offset=8, start_offset=8, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_FAST', opcode=124, arg=1, argval='b', argrepr='b', offset=10, start_offset=10, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='BUILD_TUPLE', opcode=102, arg=2, argval=2, argrepr='', offset=12, start_offset=12, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CONST', opcode=100, arg=1, argval=code_object_f, argrepr=repr(code_object_f), offset=14, start_offset=14, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='MAKE_FUNCTION', opcode=24, arg=None, argval=None, argrepr='', offset=16, start_offset=16, starts_line=None, is_jump_target=False, positions=None),
Expand All @@ -1624,10 +1624,10 @@ def _prepare_test_cases():
Instruction(opname='MAKE_CELL', opcode=135, arg=1, argval='d', argrepr='d', offset=4, start_offset=4, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='RESUME', opcode=151, arg=0, argval=0, argrepr='', offset=6, start_offset=6, starts_line=2, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CONST', opcode=100, arg=2, argval=(5, 6), argrepr='(5, 6)', offset=8, start_offset=8, starts_line=3, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CLOSURE', opcode=136, arg=3, argval='a', argrepr='a', offset=10, start_offset=10, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CLOSURE', opcode=136, arg=4, argval='b', argrepr='b', offset=12, start_offset=12, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CLOSURE', opcode=136, arg=0, argval='c', argrepr='c', offset=14, start_offset=14, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CLOSURE', opcode=136, arg=1, argval='d', argrepr='d', offset=16, start_offset=16, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_FAST', opcode=124, arg=3, argval='a', argrepr='a', offset=10, start_offset=10, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_FAST', opcode=124, arg=4, argval='b', argrepr='b', offset=12, start_offset=12, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_FAST', opcode=124, arg=0, argval='c', argrepr='c', offset=14, start_offset=14, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_FAST', opcode=124, arg=1, argval='d', argrepr='d', offset=16, start_offset=16, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='BUILD_TUPLE', opcode=102, arg=4, argval=4, argrepr='', offset=18, start_offset=18, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='LOAD_CONST', opcode=100, arg=1, argval=code_object_inner, argrepr=repr(code_object_inner), offset=20, start_offset=20, starts_line=None, is_jump_target=False, positions=None),
Instruction(opname='MAKE_FUNCTION', opcode=24, arg=None, argval=None, argrepr='', offset=22, start_offset=22, starts_line=None, is_jump_target=False, positions=None),
Expand Down
4 changes: 4 additions & 0 deletions Misc/NEWS.d/next/Core and Builtins/2023-06-24-10-34-27.gh-issue-105775.OqjoGV.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Remove the LOAD_CLOSURE opcode and make it a pseudo-op instead. This enables

* Superinstruction formation
* Removal of checks for uninitialized variables
Copy link
Copy Markdown
Member

@carljm carljm Jun 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think this PR does enable either of these things. This is because _PyCfg_OptimizeCodeUnit is called before _PyCfg_ConvertPseudoOps, in optimize_and_assemble_code_unit. When we optimize, pseudo instructions are still present.

I think this PR is still a useful incremental step (there's no reason for LOAD_CLOSURE to exist as a distinct opcode in actual bytecode or in the eval loop), but actually enabling it to participate in these optimizations will require either a) refactoring how cell/local offsets are handled in the compiler so we don't need to track the distinction between LOAD_FAST and LOAD_CLOSURE through to fix_cell_offsets, and eliminating the LOAD_CLOSURE pseudo-op entirely, or b) adding explicit LOAD_CLOSURE support to some optimizations.

We could also experiment with trying to remove pseudo-ops before optimizing, but we'd need to audit for any dependence in the optimizer on pseudo-ops, and I think currently there'd also be a problem with prepare_localsplus (which calls fix_cell_offsets) being called after optimization, and needing LOAD_CLOSURE.

Correction: I guess this PR does enable "Removal of checks for uninitialized variables," simply by virtue of unconditionally converting LOAD_CLOSURE to LOAD_FAST (not LOAD_FAST_CHECK.)

Copy link
Copy Markdown
Contributor Author

@polynomialherder polynomialherder Jun 27, 2023

Choose a reason for hiding this comment

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

Thanks, that makes sense @carljm -- I realize now that I naively copied what was in the original issue, but should have taken into account the different approach + what was happening in _PyCompile_Assemble

1 change: 1 addition & 0 deletions PC/launcher.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1270,6 +1270,7 @@ static PYC_MAGIC magic_values[] = {
/* Allow 50 magic numbers per version from here on */
{ 3450, 3499, L"3.11" },
{ 3500, 3549, L"3.12" },
{ 3550, 3599, L"3.13" },
{ 0 }
};

Expand Down
11 changes: 3 additions & 8 deletions Python/bytecodes.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ dummy_func(
// BEGIN BYTECODES //
inst(NOP, (--)) {
}

inst(RESUME, (--)) {
assert(tstate->cframe == &cframe);
assert(frame == cframe.current_frame);
Expand Down Expand Up @@ -177,12 +176,9 @@ dummy_func(
}
}

inst(LOAD_CLOSURE, (-- value)) {
/* We keep LOAD_CLOSURE so that the bytecode stays more readable. */
value = GETLOCAL(oparg);
ERROR_IF(value == NULL, unbound_local_error);
Copy link
Copy Markdown
Member

@carljm carljm Jun 26, 2023

Choose a reason for hiding this comment

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

I note that we are implicitly eliminating this NULL check by converting LOAD_CLOSURE to LOAD_FAST. I think that this is actually just fine, because ensuring this local exists (and is a cell variable) is the responsibility of the compiler, not the code author. The compiler will always insert a MAKE_CELL at the top of the function for any cell variable, so I can't see any way this null check would ever have failed. If it ever did, that would indicate a compiler bug, so it's not clear that UnboundLocalError would ever have made sense here anyway.

Copy link
Copy Markdown
Member

@gvanrossum gvanrossum Jun 27, 2023

Choose a reason for hiding this comment

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

@markshannon What do you think? We could replace it with LOAD_CLOSURE_CHECK instead to preserve this. There might be a way for a debugger to delete the variable.

Copy link
Copy Markdown
Member

@carljm carljm Jun 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

There might be a way for a debugger to delete the variable.

I don't believe there is, unless the debugger is doing something unsupported and poking directly at the fastlocals array on a _PyInterpreterFrame. The supported way for debuggers to change the value of variables is via frame.f_locals dict, which is synced back to fast locals by PyFrame_LocalsToFast, and will only update cell contents, never overwrite or delete the cell itself.

Py_INCREF(value);
}
pseudo(LOAD_CLOSURE) = {
LOAD_FAST,
};

inst(LOAD_FAST_CHECK, (-- value)) {
value = GETLOCAL(oparg);
Expand All @@ -201,7 +197,6 @@ dummy_func(
// do not use SETLOCAL here, it decrefs the old value
GETLOCAL(oparg) = NULL;
}

inst(LOAD_FAST_LOAD_FAST, ( -- value1, value2)) {
uint32_t oparg1 = oparg >> 4;
uint32_t oparg2 = oparg & 15;
Expand Down
3 changes: 2 additions & 1 deletion Python/compile.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,9 +843,10 @@ stack_effect(int opcode, int oparg, int jump)
* of __(a)enter__ and push 2 values before jumping to the handler
* if an exception be raised. */
return jump ? 1 : 0;

case STORE_FAST_MAYBE_NULL:
return -1;
case LOAD_CLOSURE:
return 1;
case LOAD_METHOD:
return 1;
case LOAD_SUPER_METHOD:
Expand Down
4 changes: 4 additions & 0 deletions Python/flowgraph.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2078,6 +2078,10 @@ _PyCfg_ConvertPseudoOps(basicblock *entryblock)
assert(SAME_OPCODE_METADATA(instr->i_opcode, NOP));
INSTR_SET_OP0(instr, NOP);
}
else if (instr->i_opcode == LOAD_CLOSURE) {
assert(SAME_OPCODE_METADATA(LOAD_CLOSURE, LOAD_FAST));
instr->i_opcode = LOAD_FAST;
}
else if (instr->i_opcode == STORE_FAST_MAYBE_NULL) {
assert(SAME_OPCODE_METADATA(STORE_FAST_MAYBE_NULL, STORE_FAST));
instr->i_opcode = STORE_FAST;
Expand Down
Loading