GeezerVison

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

GeezerVison is showing github.com — gh-98831: Modernize the FOR_ITER family of instructions by gvanrossum · Pull Request #101626 · python/cpython

Conversation

gvanrossum commented Feb 7, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

This is somewhat tricky because most of the family members (with the exception of FOR_ITER_GEN) are actually super-instructions that jump over the END_FOR instruction that is the target of the jump (which pops two stack items). In addition, FOR_ITER_RANGE also jumps over the STORE_FAST that follows it when the range is not exhausted.

The cache and stack effects documented in the DSL don't acknowledge the existence of these optimizations. (This will bite us when we try to incorporate these instructions in a level 2 interpreter, we'll have to deal with it then.)

Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum added 5 commits

February 6, 2023 14:33


          Modernize FOR_ITER

5869fba


          Modernize FOR_ITER_LIST

bd65e1c


          Modernize FOR_ITER_TUPLE

872e156


          Modernize FOR_ITER_RANGE (even weirder)

a52a79d


          Modernize FOR_ITER_GEN

53cfd22

gvanrossum requested review from Fidget-Spinner, brandtbucher and iritkatriel

February 7, 2023 00:26

bedevere-bot added the awaiting core review label

Feb 7, 2023

bedevere-bot mentioned this pull request

Feb 7, 2023

Generate the interpreter #98831

Closed

gvanrossum added the skip news label

Feb 7, 2023

Fidget-Spinner reviewed

Feb 7, 2023

View reviewed changes

Fidget-Spinner 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.

Code looks good. Just one question

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Python/generated_cases.c.h

		if (it->it_index < PyList_GET_SIZE(seq)) {
		PyObject *next = PyList_GET_ITEM(seq, it->it_index++);
		PUSH(Py_NewRef(next));
		JUMPBY(INLINE_CACHE_ENTRIES_FOR_ITER);

Fidget-Spinner Feb 7, 2023

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.

I'm a little confused, why do we not need to JUMPBY INLINE_CACHE_ENTRIES_FOR_ITER anymore?

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Feb 7, 2023

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.

I think this is the JUMPBY(1); in line 2688.

Sorry, something went wrong.

Uh oh!

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

All reactions

Fidget-Spinner Feb 7, 2023

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.

Ahh right. I forgot that the inline cache entries for FOR_ITER is just the counter. Thanks Irit!

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel reviewed

Feb 7, 2023

View reviewed changes

Comment thread

Python/bytecodes.c

		FOR_ITER_GEN,
		};

		inst(FOR_ITER, (unused/1, iter -- iter, next)) {

iritkatriel Feb 7, 2023

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.

Should this be counter/1 instead of unused/1, and then the counter used in the body?

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Feb 7, 2023

Copy link

Copy Markdown

Member Author

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.

In all cases where the counter is manipulated using ADAPTIVE_COUNTER_IS_ZERO / DECREMENT_ADAPTIVE_COUNTER I'm leaving this idiom unchanged -- the generator won't let you write back to cache entries, so it would become asymmetrical, and the common case is DECREMENT_ADAPTIVE_COUNTER anyway.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel approved these changes

Feb 7, 2023

View reviewed changes

Comment thread

Python/bytecodes.c Resolved

Uh oh!

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

bedevere-bot added awaiting merge and removed awaiting core review labels

Feb 7, 2023

gvanrossum and others added 2 commits

February 7, 2023 07:11


          Accept comment suggestion

76a4a7b

Co-authored-by: Irit Katriel <[email protected]>


          Update generated file

b39cf90

gvanrossum merged commit d54b8d8 into python:main

Feb 7, 2023

bedevere-bot removed the awaiting merge label

Feb 7, 2023

gvanrossum deleted the for-iter-family branch

February 7, 2023 16:28

Reviewers

Fidget-Spinner

Fidget-Spinner left review comments

iritkatriel

iritkatriel approved these changes

brandtbucher

Awaiting requested review from brandtbucher

Uh oh!

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

Assignees

No one assigned

Labels

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.

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

GeezerVison

Conversation

gvanrossum commented Feb 7, 2023 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Fidget-Spinner left a comment

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

iritkatriel Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Fidget-Spinner Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

iritkatriel Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

gvanrossum Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gvanrossum commented Feb 7, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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