GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — GH-123391: Code generators: Track stack and locals in `if` statements. by markshannon · Pull Request #123397 · python/cpython

Conversation

markshannon commented Aug 27, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This PR adds little functionality, but it will enable us to do much more effective stack spilling in the future.

I've deliberately left the spilling functionality for a later PR to make this PR easier to review.

This PR:

Adds a new Storage class which combines the state of the stack and the local variables within a micro-op.
Adds copying and merging functionality to that class, and the underlying stack and locals, to support tracking state across divergent flow.
Splits and merges the storage in if statements.

The resulting generated code is basically the same as we have now.

To simplify parsing, the code generator also enforces PEP 7 rules for braces.
Most of the changes in bytecodes.c are a result of this change.

Issue: Interpreter code generators need to be able to handle basic flow control #123391

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon added 14 commits

August 7, 2024 16:13


          Add copy and == support to Stack class

75bda28


          Merge branch 'main' into stack-copy-and-merge

c7e9102


          blacken stack.py

d331371


          Cases generator: Track reachability and divergent stacks in if statem…

4673d8a

…ents.


          Fix type errors and rename ahead to look_ahead

132df06


          Track state of output variables

b7f71d4


          Handle stack and output locals togather as Storage class

b97dea9


          Track locals as well as stack on differing paths

9838a05


          Use 'PEP 7' in syntax error, to make it clear that this is not a C sy…

1f829be

…ntax error, just a tool limitation.


          Push peeks back to stack in optimizer code gen

d2e5f12


          Merge branch 'main' into stack-copy-and-merge


          Update test

1754fc4


          Cleanup whitespace

98f9720


          Add tests for PEP 7 parsing and escaping call in condition

0284b3f

markshannon requested a review from Fidget-Spinner as a code owner

August 27, 2024 14:54

bedevere-app Bot mentioned this pull request

Aug 27, 2024

Interpreter code generators need to be able to handle basic flow control #123391

Closed

bedevere-app Bot added the awaiting core review label

Aug 27, 2024

markshannon added the skip news label

Aug 27, 2024

markshannon added 2 commits

August 27, 2024 16:50


          Remove merge artifact

03bea71


          Remove test for escaping calls in conditions

ca2f457

picnixz reviewed

Aug 27, 2024

View reviewed changes

picnixz left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Some nitpicks.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/stack.py

Comment on lines +426 to +427

		f"Locals not defined in stack order. "
		f"Expected '{out.name}' is defined before '{undefined}'"

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	f"Locals not defined in stack order. "
	f"Expected '{out.name}' is defined before '{undefined}'"
	f"Locals not defined in stack order. "
	f"Expected {out.name!r} to be defined before {undefined!r}"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/stack.py

		break
		undefined = ""
		for out in self.outputs:
		if out.defined:

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If the first out.defined holds, then we don't really have anything "before" (unless this case is not possible?)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/tier2_generator.py

		self.emit(next(tkn_iter))
		lparen = next(tkn_iter)
		self.emit(lparen)
		assert(lparen.kind == "LPAREN")

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert(lparen.kind == "LPAREN")
	assert lparen.kind == "LPAREN"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/tier2_generator.py

		self.emit(next(tkn_iter))
		lparen = next(tkn_iter)
		self.emit(lparen)
		assert(lparen.kind == "LPAREN")

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert(lparen.kind == "LPAREN")
	assert lparen.kind == "LPAREN"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/generators_common.py

		self.out.emit(next(tkn_iter))
		lparen = next(tkn_iter)
		self.emit(lparen)
		assert(lparen.kind == "LPAREN")

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert(lparen.kind == "LPAREN")
	assert lparen.kind == "LPAREN"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/generators_common.py

Comment on lines +111 to +115

		raise analysis_error(
		f"Locals not defined in stack order. "
		f"Expected '{out.name}' is defined before '{undefined}'",
		tkn
		)

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	raise analysis_error(
	f"Locals not defined in stack order. "
	f"Expected '{out.name}' is defined before '{undefined}'",
	tkn
	)
	raise analysis_error(
	f"Locals not defined in stack order. "
	f"Expected {out.name!r} to be defined before {undefined!r}",
	tkn
	)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/stack.py

		other.top_offset = self.top_offset.copy()
		other.base_offset = self.base_offset.copy()
		other.variables = [var.copy() for var in self.variables]
		other.defined = set(self.defined)

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	other.defined = set(self.defined)
	other.defined = self.defined.copy()

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/cases_generator/generators_common.py

		self.out.emit(next(tkn_iter))
		lparen = next(tkn_iter)
		self.emit(lparen)
		assert(lparen.kind == "LPAREN")

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change

	assert(lparen.kind == "LPAREN")
	assert lparen.kind == "LPAREN"

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/test/test_generated_cases.py

		self.run_cases_test(input, output)


		def test_pep7_condition(self):

picnixz Aug 27, 2024

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Maybe test the else branch as well?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

markshannon mentioned this pull request

Sep 23, 2024

GH-119866: Spill before escaping calls in all cases. #124392

Merged

markshannon commented Sep 23, 2024

Copy link

Copy Markdown

Member Author

Replaced by #124392

@picnixz Thanks for review. I've ported your suggestion to the replacement PR.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon closed this

Sep 23, 2024

Reviewers

picnixz

picnixz left review comments

Fidget-Spinner

Awaiting requested review from Fidget-Spinner

Fidget-Spinner is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

markshannon commented Aug 27, 2024 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markshannon commented Sep 23, 2024

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

markshannon commented Aug 27, 2024 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.