GeezerVison

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

GeezerVison is showing github.com — gh-105481: remove regen-opcode. Generated _PyOpcode_Caches in regen-cases. by iritkatriel · Pull Request #108367 · python/cpython

Conversation

iritkatriel commented Aug 23, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

Fixes #105481 .

Issue: Generate opcode metadata from bytecodes.c instead of opcode.py #105481

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel added 2 commits

August 23, 2023 14:52


          pythongh-105481: remove regen-opcode. Generated _PyOpcode_Caches in r…

fb23d1b

…egen-cases.


          add news

fec7cde

iritkatriel requested review from a team and markshannon as code owners

August 23, 2023 13:56

bedevere-bot mentioned this pull request

Aug 17, 2023

Generate opcode metadata from bytecodes.c instead of opcode.py #105481

Closed

bedevere-bot added the awaiting core review label

Aug 23, 2023

iritkatriel commented

Aug 23, 2023

View reviewed changes

Comment thread

Tools/cases_generator/generate_cases.py

		if instr.cache_offset > 0:
		self.out.emit(f'[{name}] = {instr.cache_offset},')
		# Irregular case:
		self.out.emit('[JUMP_BACKWARD] = 1,')

iritkatriel Aug 23, 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.

How should we deal with this case?

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Aug 23, 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.

Put JUMP_BACKWARD in a family of its own, and add the cache effect notation to its instr? Or is that a problem because it's also part of the JUMP pseudo?

If it's complicated I'm fine with leaving this special case.

Sorry, something went wrong.

Uh oh!

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

iritkatriel reacted with thumbs up emoji

All reactions

👍 1 reaction

gvanrossum Aug 23, 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.

Alas, it looks like it's in a pseudo with JUMP_FORWARD and those are required to have the same size. Maybe that requirement is too strict?

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Aug 23, 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 an offline discussion with @markshannon we decided to add a family for JUMP_BACKWARD which would declare its cache size. This causes the generator to emit an assertion that the cache size declared for the family (1) is equal to that which is calculated from the instruction signature (0). If I add the cache to the signature, then I get an error about the pseudo-instructions for JUMP (JUMP_FORWARD and JUMP_BACKWARD) not having the same instruction format.

So there is something here that we need to resolve, and it's not a trivial change. I suggest we commit this PR (which is large enough) with the workaround on line 553, and resolve it in a separate PR.

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Aug 23, 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.

Agreed. LGTM.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Aug 23, 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.

Ah, I didn't see your comment before I wrote this ^. Yes, we seem to agree.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel requested a review from gvanrossum

August 23, 2023 13:58

iritkatriel added 2 commits

August 23, 2023 15:12


          add missing entries to makefile's headers list

8c4aa3f


          Merge remote-tracking branch 'upstream/main' into caches

9f044f0

gvanrossum approved these changes

Aug 23, 2023

View reviewed changes

Comment thread

Tools/cases_generator/generate_cases.py Resolved

Uh oh!

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

Comment thread

Tools/cases_generator/generate_cases.py

		if instr.cache_offset > 0:
		self.out.emit(f'[{name}] = {instr.cache_offset},')
		# Irregular case:
		self.out.emit('[JUMP_BACKWARD] = 1,')

gvanrossum Aug 23, 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.

Put JUMP_BACKWARD in a family of its own, and add the cache effect notation to its instr? Or is that a problem because it's also part of the JUMP pseudo?

If it's complicated I'm fine with leaving this special case.

Sorry, something went wrong.

Uh oh!

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

iritkatriel reacted with thumbs up emoji

All reactions

👍 1 reaction

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

Aug 23, 2023


          use _

f2e82d3

Co-authored-by: Guido van Rossum <[email protected]>

iritkatriel merged commit 72119d1 into python:main

Aug 23, 2023

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

Aug 23, 2023

bedevere-bot commented Aug 23, 2023

Copy link

Copy Markdown

There's a new commit after the PR has been approved.

@gvanrossum: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

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

bedevere-bot requested a review from gvanrossum

August 23, 2023 17:39

Reviewers

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

gvanrossum

Awaiting requested review from gvanrossum

Uh oh!

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

Assignees

No one assigned

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Generate opcode metadata from bytecodes.c instead of opcode.py

Uh oh!

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

3 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

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

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 23, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

iritkatriel commented Aug 23, 2023 •

edited by bedevere-bot

Loading

Uh oh!

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