gh-105775: Convert LOAD_CLOSURE to a pseudo-op by polynomialherder · Pull Request #106059 · python/cpython

polynomialherder · 2023-06-24T17:55:32Z

Per #105775 this change converts LOAD_CLOSURE from an opcode to a pseudo-opcode. This enables removal of checks for uninitialized variables, and frees up an instruction.

Issue: Remove LOAD_CLOSURE #105775

📚 Documentation preview 📚: https://cpython-previews--106059.org.readthedocs.build/

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.

ghost · 2023-06-24T17:56:01Z

All commit authors signed the Contributor License Agreement.

bedevere-bot · 2023-06-24T20:35:14Z

🤖 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.

carljm · 2023-06-26T15:31:54Z

Doc/library/dis.rst

+   storage.
+
+   Note that ``LOAD_CLOSURE`` is replaced with ``LOAD_FAST`` in the assembler.
+   It exists to keep bytecode a little more readable.


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.

carljm · 2023-06-26T15:44:29Z

Misc/NEWS.d/next/Core and Builtins/2023-06-24-10-34-27.gh-issue-105775.OqjoGV.rst

+
+* Superinstruction formation
+* Removal of checks for uninitialized variables


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.)

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

carljm · 2023-06-26T15:57:59Z

Python/bytecodes.c

-        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);


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.

@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 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

carljm

LGTM. Thanks @polynomialherder !

carljm · 2023-06-27T16:49:21Z

@polynomialherder can you merge in main again to resolve the conflict?

gvanrossum

I recommend @iritkatriel review this as well.

Python/bytecodes.c

gvanrossum · 2023-06-27T17:31:09Z

Python/bytecodes.c

-        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);


@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.

Python/bytecodes.c

Python/compile.c

iritkatriel

Let's not insult pseudo ops by calling this a demotion. Pseudo ops are awesome.

iritkatriel · 2023-06-27T17:40:12Z

Doc/library/dis.rst

+   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


Suggested change

This opcode was demoted from full opcode to pseudo-instruction

This opcode is now a pseudo-instruction.

Lib/opcode.py

Lib/test/test_compiler_assemble.py

Misc/NEWS.d/next/Core and Builtins/2023-06-24-10-34-27.gh-issue-105775.OqjoGV.rst

Lib/importlib/_bootstrap_external.py

- 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

polynomialherder · 2023-06-27T23:19:46Z

Thank you @iritkatriel and @gvanrossum for the thorough reviews -- I think I worked in those changes, let me know if I missed anything

Misc/NEWS.d/next/Core and Builtins/2023-06-24-10-34-27.gh-issue-105775.OqjoGV.rst

gvanrossum

Looks good to me. Thanks for your contribution -- this is good work, and we're looking forward to your next project!

pythongh-105775: Demote LOAD_CLOSURE to a pseudo-op

21d086e

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.

polynomialherder requested review from a team, brettcannon, ericsnowcurrently, iritkatriel, markshannon, ncoghlan and warsaw as code owners June 24, 2023 17:55

bedevere-bot added the awaiting review label Jun 24, 2023

bedevere-bot mentioned this pull request Jun 24, 2023

Remove LOAD_CLOSURE #105775

Closed

pythongh-106044: Fix whitespace

6921930

gvanrossum added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 24, 2023

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Jun 24, 2023

polynomialherder and others added 2 commits June 24, 2023 19:42

pythongh-105775: Regenerate generated_cases.c.h

b111d10

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

bbdbb99

carljm reviewed Jun 26, 2023

View reviewed changes

polynomialherder added 3 commits June 26, 2023 19:14

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

32c1a70

Merge branch 'Remove-LOAD-CLOSURE-105775' of github.com:polynomialher…

4103b9a

…der/cpython into Remove-LOAD-CLOSURE-105775

carljm approved these changes Jun 27, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting review labels Jun 27, 2023

polynomialherder added 2 commits June 27, 2023 10:51

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

269933e

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

a069593

gvanrossum reviewed Jun 27, 2023

View reviewed changes

iritkatriel reviewed Jun 27, 2023

View reviewed changes

iritkatriel changed the title ~~gh-105775: Demote LOAD_CLOSURE to a pseudo-op~~ gh-105775: Convert LOAD_CLOSURE to a pseudo-op Jun 27, 2023

polynomialherder added 2 commits June 27, 2023 16:42

pythongh-105775: Update documentation, whitespace tweaks

988189d

- 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

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

968601d

brettcannon removed their request for review June 27, 2023 22:54

iritkatriel reviewed Jun 28, 2023

View reviewed changes

Misc/NEWS.d/next/Core and Builtins/2023-06-24-10-34-27.gh-issue-105775.OqjoGV.rst Outdated Show resolved Hide resolved

iritkatriel approved these changes Jun 28, 2023

View reviewed changes

polynomialherder added 3 commits June 28, 2023 07:35

Update news item

46fa9d5

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

2b677b6

Merge branch 'main' into Remove-LOAD-CLOSURE-105775

ab8ce45

polynomialherder requested a review from gvanrossum June 29, 2023 14:09

gvanrossum approved these changes Jun 29, 2023

View reviewed changes

gvanrossum merged commit 8bff940 into python:main Jun 29, 2023

bedevere-bot removed the awaiting merge label Jun 29, 2023

polynomialherder deleted the Remove-LOAD-CLOSURE-105775 branch June 30, 2023 01:41

polynomialherder mentioned this pull request Jul 7, 2023

Docs: Description of LOAD_CLOSURE in dis module should be updated #102636

Closed


		* Superinstruction formation
		* Removal of checks for uninitialized variables

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

Uh oh!

Conversation

polynomialherder commented Jun 24, 2023 • edited by iritkatriel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 24, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jun 24, 2023

Uh oh!

carljm Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

carljm Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

polynomialherder Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

carljm Jun 26, 2023

Choose a reason for hiding this comment

Uh oh!

gvanrossum Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

carljm Jun 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carljm left a comment

Choose a reason for hiding this comment

Uh oh!

carljm commented Jun 27, 2023

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gvanrossum Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

iritkatriel left a comment

Choose a reason for hiding this comment

Uh oh!

iritkatriel Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polynomialherder commented Jun 27, 2023

Uh oh!

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

polynomialherder commented Jun 24, 2023 •

edited by iritkatriel

Loading

ghost commented Jun 24, 2023 •

edited by ghost

Loading

carljm Jun 26, 2023 •

edited

Loading

carljm Jun 27, 2023 •

edited

Loading