GeezerVison

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

GeezerVison is showing github.com — gh-98831: Use opcode metadata for stack_effect() by gvanrossum · Pull Request #101704 · python/cpython

Conversation

gvanrossum commented Feb 8, 2023 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

In order to do this, I had to change the generated opcode_metadata.h file to be sensitive to whether NEED_OPCODE_TABLES is defined -- if it is, it defines the functions and data with public linkage, if it isn't it only declares them. That way the file can be included in multiple places without worry about duplicating static data or functions.

Also, the pushed/popped functions will now accept any integer opcode argument and return -1 for unknown opcodes.

This also changes the command line options for generate_cases.py to write all output files in a single run, and updates Makefile.pre.in accordingly. This halves the time to run the cases generator (since most of the time goes into parsing bytecodes.c).

Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

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

brandtbucher reacted with hooray emoji

All reactions

🎉 1 reaction

bedevere-bot added the awaiting core review label

Feb 8, 2023

bedevere-bot mentioned this pull request

Feb 8, 2023

Generate the interpreter #98831

Closed

gvanrossum added the skip news label

Feb 8, 2023

iritkatriel reviewed

Feb 8, 2023

View reviewed changes

Comment thread

Makefile.pre.in Resolved

Uh oh!

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

gvanrossum added 7 commits

February 8, 2023 15:15


          Write output and metadata in a single run

f59751d

This halves the time to run the cases generator
(most of the time goes into parsing the input).


          Declare or define opcode metadata based on NEED_OPCODE_TABLES

383c1fc


          Use generated metadata for stack_effect()

c5db6bc


          compile.o depends on opcode_metadata.h

683d8f7


          Return -1 for unknown opcode

a271ebc


          Support jump<0 in stack_effect()

977e639


          stack_effect() for specialized opcode is an error

bd19d79

gvanrossum force-pushed the cases-infra branch from 8d5a6c4 to bd19d79 Compare

February 8, 2023 23:16

gvanrossum marked this pull request as ready for review

February 8, 2023 23:16

gvanrossum requested a review from markshannon as a code owner

February 8, 2023 23:16

gvanrossum requested a review from iritkatriel

February 8, 2023 23:17

iritkatriel approved these changes

Feb 8, 2023

View reviewed changes

Comment thread

Python/compile.c Outdated


		case LOAD_BUILD_CLASS:
		return 1;
		if (0 <= opcode && opcode < 256) {

iritkatriel Feb 8, 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.

We have these:

#define MAX_REAL_OPCODE 254
 
#define IS_WITHIN_OPCODE_RANGE(opcode) \
        (((opcode) >= 0 && (opcode) <= MAX_REAL_OPCODE) || \
         IS_PSEUDO_OPCODE(opcode))

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Feb 8, 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.

Okay, I'll use opcode <= MAX_REAL_OPCODE. I feel we don't need IS_PSEUDO_OPCODE() because the switch will take care of that.

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

Comment thread

Python/compile.c

		if (0 <= opcode && opcode < 256) {
		if (_PyOpcode_Deopt[opcode] != opcode) {
		// Specialized instructions are not supported.
		return PY_INVALID_STACK_EFFECT;

iritkatriel Feb 8, 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.

Wouldn't it be correct to return the stack effect of _PyOpcode_Deopt[opcode]?

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Feb 8, 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.

Wouldn't it be correct to return the stack effect of _PyOpcode_Deopt[opcode]?

I thought so, but there are unit tests that insist that this is an error, and I didn't feel like changing the tests:

        # All not defined opcodes
        for code in set(range(256)) - set(dis.opmap.values()):
            with self.subTest(opcode=code):
                self.assertRaises(ValueError, stack_effect, code)
                self.assertRaises(ValueError, stack_effect, code, 0)

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Feb 8, 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.

Ah, those tests probably predate specialization. We don't need to change this now.

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Feb 8, 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.

opcode.py goes to great lengths to hide the existence of specialized instructions -- they don't show in either opname or opmap, even though pseudo ops do exist there. So I think it's by design. Though we could argue for changing that design.

I found the algorithm in dis.py on L46-52. I think it's duplicated in Tools/build/generate_opcode_h.py on L145-151.

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Feb 9, 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 dis.py:46-52 is actually allocating unused opcodes for the specialised instructions, while generate_opcode_h.py:145-151 is building the full deopt lookup table from opcode["_specializations"], plus mapping each normal opcode to itself.

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Feb 9, 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.

Oh wait, in generate_opcode_h.py it's L101-107. These two algorithms ought to match, otherwise results will be hilarious. :-)

Anyway, I'm going to merge now. On to figuring out whether mark_stacks() is in reach yet (I doubt it).

Sorry, something went wrong.

Uh oh!

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

All reactions

iritkatriel Feb 9, 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.

Ah yes. Wow, we need to fix that.

Sorry, something went wrong.

Uh oh!

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

All reactions

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

Feb 8, 2023


          Use MAX_REAL_OPCODE as upper opcode limit

658ea17

gvanrossum merged commit 65b7b6b into python:main

Feb 9, 2023

bedevere-bot removed the awaiting merge label

Feb 9, 2023

gvanrossum deleted the cases-infra branch

February 9, 2023 00:23

carljm added a commit to carljm/cpython that referenced this pull request

Feb 9, 2023


          Merge branch 'main' into inlinecomp2

be3becc

* main: (82 commits)
  pythongh-101670: typo fix in PyImport_ExtendInittab() (python#101723)
  pythonGH-99293: Document that `Py_TPFLAGS_VALID_VERSION_TAG` shouldn't be used. (#pythonGH-101736)
  no-issue: Add Dong-hee Na as the cjkcodecs codeowner (pythongh-101731)
  pythongh-101678: Merge math_1_to_whatever() and math_1() (python#101730)
  pythongh-101678: refactor the math module to use special functions from c11 (pythonGH-101679)
  pythongh-85984: Remove legacy Lib/pty.py code. (python#92365)
  pythongh-98831: Use opcode metadata for stack_effect() (python#101704)
  pythongh-101283: Version was just released, so should be changed in 3.11.3 (pythonGH-101719)
  pythongh-101283: Fix use of unbound variable (pythonGH-101712)
  pythongh-101283: Improved fallback logic for subprocess with shell=True on Windows (pythonGH-101286)
  pythongh-101277: Port more itertools static types to heap types (python#101304)
  pythongh-98831: Modernize CALL and family (python#101508)
  pythonGH-101696: invalidate type version tag in `_PyStaticType_Dealloc` (python#101697)
  pythongh-100221: Fix creating dirs in `make sharedinstall` (pythonGH-100329)
  pythongh-101670: typo fix in PyImport_AppendInittab() (pythonGH-101672)
  pythongh-101196: Make isdir/isfile/exists faster on Windows (pythonGH-101324)
  pythongh-101614: Don't treat python3_d.dll as a Python DLL when checking extension modules for incompatibility (pythonGH-101615)
  pythongh-100933: Improve `check_element` helper in `test_xml_etree` (python#100934)
  pythonGH-101578: Normalize the current exception (pythonGH-101607)
  pythongh-47937: Note that Popen attributes are read-only (python#93070)
  ...

Reviewers

iritkatriel

iritkatriel approved these changes

markshannon

Awaiting requested review from markshannon

markshannon is a code owner

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.

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

gvanrossum commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gvanrossum commented Feb 8, 2023 •

edited

Loading

Uh oh!

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