gh-105775: Convert LOAD_CLOSURE to a pseudo-op #106059
Conversation
This change demotes LOAD_CLOSURE from an instruction to a pseudo-instruction. This enables superinstruction formation, removal of checks for uninitialized variables, and frees up an instruction.
|
All commit authors signed the Contributor License Agreement. |
|
🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 6921930 🤖 If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again. |
Doc/library/dis.rst
Outdated
| storage. | ||
|
|
||
| Note that ``LOAD_CLOSURE`` is replaced with ``LOAD_FAST`` in the assembler. | ||
| It exists to keep bytecode a little more readable. |
There was a problem hiding this comment.
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.
|
|
||
| * Superinstruction formation | ||
| * Removal of checks for uninitialized variables |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
This change adds a unit test for assembly of code objects containing the LOAD_CLOSURE pseud-instruction We also correct the documentation and NEWS descriptions to more accurately reflect the effects of converting LOAD_CLOSURE to a pseudo-op and the reason for its existence in the first place
…der/cpython into Remove-LOAD-CLOSURE-105775
|
@polynomialherder can you merge in main again to resolve the conflict? |
| 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); |
There was a problem hiding this comment.
@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.
Doc/library/dis.rst
Outdated
| Note that ``LOAD_CLOSURE`` is replaced with ``LOAD_FAST`` in the assembler. | ||
|
|
||
| .. versionchanged:: 3.13 | ||
| This opcode was demoted from full opcode to pseudo-instruction |
There was a problem hiding this comment.
| This opcode was demoted from full opcode to pseudo-instruction | |
| This opcode is now a pseudo-instruction. |
Misc/NEWS.d/next/Core and Builtins/2023-06-24-10-34-27.gh-issue-105775.OqjoGV.rst
Outdated
Show resolved
Hide resolved
- Describe the LOAD_CLOSURE change using more neutral language - Add back in deleted whitespace - Add an assertion to IsolatedAssembleTests.test_expression_with_pseudo_instruction_load_closure - Regenerate generated files
|
Thank you @iritkatriel and @gvanrossum for the thorough reviews -- I think I worked in those changes, let me know if I missed anything |
Per #105775 this change converts
LOAD_CLOSUREfrom an opcode to a pseudo-opcode. This enables removal of checks for uninitialized variables, and frees up an instruction.LOAD_CLOSURE#105775📚 Documentation preview 📚: https://cpython-previews--106059.org.readthedocs.build/