GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-98831: Implement super-instruction generation by gvanrossum · Pull Request #99084 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Nov 4, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@markshannon Please see "PLEASE NOTE" below.

This is pretty straightforward: the expansion of

super(FOO__BAR) = FOO + BAR;

generates code like this:

TARGET(FOO__BAR) {
    {
        <code block for FOO>
    }
    NEXTOPARG();
    next_instr++;
    {
        <code block for BAR>
    }
    DISPATCH();
}

The only tricky thing is that the code block for LOAD_CONST contains a PREDICTED() macro call, so we strip ththoseat out. (Eventually we'll auto-generate the PREDICTED() macros so we won't need to do this any more, but super-instruction generation is a simpler project than auto-PREDICTED insertion, so I figured I'd tackle it first.)

The new super-instructions are all placed at the end (the parser doesn't keep track of how instruction definitions are interspersed), so the diff for generated_cases.c.h looks perhaps a bit more imposing than it is.

PLEASE NOTE: The code for some of the generated super-instructions is different than the hand-written super-instructions because somehow the hand-written versions moved the NEXTOPARG(); next_instr++; pair up a few lines. I don't know why the originals were written that way -- @markshannon do you remember? Do you think it matters? (Another difference is that the parts each define their own variables, whereas the hand-written versions reused PyObject *value, but I assume the optimizer will handle that.)

  • Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

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

All reactions
@gvanrossum
Implement super-instruction generation
55816fd
@bedevere-bot bedevere-bot mentioned this pull request Nov 4, 2022
Generate the interpreter #98831
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 4, 2022
@gvanrossum
Update NEWS for issue 98831
0d746d2
@gvanrossum

gvanrossum commented Nov 4, 2022

Copy link
Copy Markdown
Member Author

@CAM-Gerlach Can you double check the markup in the NEWS entry? What's the recommended way to refer to a markdown file in a different repo owned by a different org?

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum
gvanrossum requested review from brandtbucher and markshannon November 4, 2022 01:48
CAM-Gerlach
CAM-Gerlach reviewed Nov 4, 2022
View reviewed changes

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

I had a few small suggestions to clarify the news entry and move the link to be inline where the DSL introduced, instead of a bare URL at the end of the body, but nothing too major.

Maybe out of scope for this particular PR, but while it may not affect too many people's work directly, this #98831 overall seems a significant enough change to deserve a What's New entry in the (e.g. in the Build Section, or as appropriate), which this NEWS entry seems like a great basis for.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Misc/NEWS.d/next/Build/2022-10-28-22-24-26.gh-issue-98831.IXRCRX.rst Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Build/2022-10-28-22-24-26.gh-issue-98831.IXRCRX.rst Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Build/2022-10-28-22-24-26.gh-issue-98831.IXRCRX.rst Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Build/2022-10-28-22-24-26.gh-issue-98831.IXRCRX.rst Resolved

Uh oh!

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

Comment thread Misc/NEWS.d/next/Build/2022-10-28-22-24-26.gh-issue-98831.IXRCRX.rst Resolved

Uh oh!

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

@CAM-Gerlach

CAM-Gerlach commented Nov 4, 2022

Copy link
Copy Markdown
Member

What's the recommended way to refer to a markdown file in a different repo owned by a different org?

Right now, same or different repo/org, there isn't (AFAIK) currently an easy way to do this with a nice role as opposed to just a plain reST link.

However, following discussion and some experimentation in the devguide repo and Discord, I'm working on a little Sphinx extension with a base "SmarkLink" role that, with just a couple lines of customization can be used to link files on the CPython repo, other repos/orgs or even other websites (like PEPs, RFCs, etc, as a smarter and more powerful version of the built-in PEP/RFC roles). If we implement that, you'll be able to do e.g. :gh-file:`Tools/cases_generator` to generate Tools/case-generator (for the current branch by default), and also supports files on other repos, e.g. :gh-file:`faster-cpython/ideas:3.12/interpreter_definition.md`, as well as specifying branch/tag/commit, standard Sphinx ~, ! and <> syntax, # for subsections, etc.

Here's an earlier prototype.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum @CAM-Gerlach
Apply suggestions from CAM's review to NEWS
7998877
Co-authored-by: C.A.M. Gerlach <[email protected]>
@gvanrossum

gvanrossum commented Nov 4, 2022

Copy link
Copy Markdown
Member Author

Right now, same or different repo/org, there isn't (AFAIK) currently an easy way to do this with a nice role as opposed to just a plain reST link.

Thanks for your suggestions! All applied.

I'm looking forward to using your "SnarkLink" extension next time. :-)

CAM-Gerlach reacted with thumbs up emoji CAM-Gerlach and AlexWaygood reacted with laugh emoji
All reactions
  • 👍 1 reaction
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 4, 2022

Copy link
Copy Markdown
Member

We need the code generator to have an understanding of stack effects before breaking instructions into their parts.
Consider a hypothetical LOAD_FAST__STORE_FAST
Simple concatenation would produce this:

TARGET(STORE_FAST__LOAD_FAST) {
    {
        PyObject *value = GETLOCAL(oparg);
        assert(value != NULL);
        Py_INCREF(value);
        PUSH(value);
    }
    NEXTOPARG();
    next_instr++;
    {
        PyObject *value = POP();
        SETLOCAL(oparg, value);
    }
    DISPATCH();
}

This is clearly inefficient.
The output should look something like:

TARGET(STORE_FAST__LOAD_FAST) {
    PyObject *_stack_tmp1;
    {
        PyObject *value = GETLOCAL(oparg);
        assert(value != NULL);
        Py_INCREF(value);
        _stack_tmp1 = value;
    }
    NEXTOPARG();
    next_instr++;
    {
        PyObject *value = _stack_tmp1
        SETLOCAL(oparg, value);
    }
    DISPATCH();
}

In order to do this, we first need to have stack effects that the tooling understands.

This probably doesn't matter for superinstructions, but does matter if we want to split specialized instructions into guard and action parts.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 4, 2022 •
edited by gvanrossum
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I think the next enhancements to generated code should be generating the code for dispatch related macros.

  • Strip out remaining DISPATCH() macros from bytecodes.c
  • Strip out all PREDICTED() macros. The code generator can insert them as required by PREDICT(name) macros.
  • Replace all gotos in bytecodes.c with ERROR, UNWIND, RESUME_FRAME, etc., so that they are easily identifiable by code-gen. This is necessary, so that the code generator can ensure that the stack in consistent in case of error or unwind. It will also allow us to experiment with different layouts and inlining of code sequences.

Implementing stack effects can be done before or after the above.

Our goal is to expose "micro ops" for optimization:
E.g.

op(FLOAT_CHECK2, (left right -- left right)) {
    DEOPT_IF(!PyFloat_CheckExact(left));
    DEOPT_IF(Py_TYPE(right) != Py_TYPE(left));
}

op(FLOAT_ADD, (left right  -- sum)) {
    STAT_INC(BINARY_OP, hit);
    double dsum = ((PyFloatObject *)left)->ob_fval +
        ((PyFloatObject *)right)->ob_fval;
    PyObject *sum = PyFloat_FromDouble(dsum);
    if (sum == NULL) {
        ERROR();
    }
    _Py_DECREF_SPECIALIZED(right, _PyFloat_ExactDealloc);
    _Py_DECREF_SPECIALIZED(left, _PyFloat_ExactDealloc);
}

BINARY_OP_ADD_FLOAT = counter/1 + FLOAT_CHECK2 + FLOAT_ADD;

And be able to generate code for BINARY_OP_ADD_FLOAT that is at least as efficient as what we have now.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Nov 4, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@markshannon, I asked you one question you didn't answer yet: Does it matter that the placement of the NEXTOPARG(); next_instr++; pairs in the generated super-instructions is different than it was in the hand-written versions? (You can compare them in the diff.)

I think the next enhancements to generated code should be generating the code for dispatch related macros.

  • Strip out remaining DISPATCH() macros from bytecodes.c

I imagine this is important for opcodes that can be combined into others. It doesn't matter for top-level instructions, right?

Doing this everywhere seems complicated, since there are quite a few that have conditional DISPATCH() all over the place (e.g. UNARY_NOT). What do you propose I do in those cases? Add a label at the end of the instruction and goto there? That seems to defeat half the purpose of DISPATCH(). (There are also easy cases, e.g. for some reason the extract_cases script didn't elide the DISPATCH() at the end of STOPITERATION_ERROR. Those I can do of course.)

  • Strip out all PREDICTED() macros. The code generator can insert them as required by PREDICT(name) macros.

That was going to be my next project, indeed.

  • Replace all gotos in bytecodes.c with ERROR, UNWIND, RESUME_FRAME, etc., so that they are easily identifiable by code-gen. This is necessary, so that the code generator can ensure that the stack in consistent in case of error or unwind. It will also allow us to experiment with different layouts and inlining of code sequences.

What purpose does this serve? The generator can recognize goto error just as easily.

Anyway, my own planned sequence of projects is roughly:

  • Complete this PR
  • Auto-generate PREDICTED() -- GH-98831: Auto-generate PREDICTED() macro calls #99102
  • Simple stack effects (no arrays, no conditions, no cache effects)

After that we should decide what's next, e.g. combining uops into instrs, cache effects, array stack effects, conditional stack effects, register opcodes, who knows.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 4, 2022

Copy link
Copy Markdown
Member

Does it matter that the placement of the NEXTOPARG(); next_instr++; pairs in the generated super-instructions is different than it was in the hand-written versions?

Only if it impairs performance. I don't think it should.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 4, 2022

Copy link
Copy Markdown
Member

We need to implement stack effects before superinstructions. Without the stack effects the code generator cannot efficiently move values from one op to the next.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 4, 2022

Copy link
Copy Markdown
Member

The generator can recognize goto error just as easily.

Sure, it is just about of consistency. It is easier to see what's going on if SHOUTY_CASE_MACROS need to be handled by the code generator, and everything can be treated as an opaque blob.

All reactions

Sorry, something went wrong.

Uh oh!

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

@markshannon

markshannon commented Nov 4, 2022

Copy link
Copy Markdown
Member

Replacing all the DISPATCH()s with gotos to the end of the instruction means that the code generator doesn't need to handle DISPATCH. It should be fine to leave it for now, as the code generator doesn't do anything with the DISPATCH() anyway.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Nov 4, 2022

Copy link
Copy Markdown
Member Author

We need to implement stack effects before superinstructions. Without the stack effects the code generator cannot efficiently move values from one op to the next.

Okay, but the current crop of 5 superinstructions don't need this. There's not a single case where the second half does something with the result of the first case (no LOAD_FAST__STORE_FAST).

All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum added 3 commits November 4, 2022 09:49
@gvanrossum
Fix whitespace
9072291
@gvanrossum
Merge branch 'main' into super-instrs
b84ee0a
@gvanrossum
Remove debug print
b5fe9fa
@gvanrossum
gvanrossum merged commit 7dcd28e into python:main Nov 6, 2022
@bedevere-bot bedevere-bot removed the awaiting core review label Nov 6, 2022
@gvanrossum
gvanrossum deleted the super-instrs branch November 6, 2022 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments
@brandtbucher brandtbucher Awaiting requested review from brandtbucher
@markshannon 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

None yet

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

@gvanrossum @CAM-Gerlach @markshannon @bedevere-bot
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.