GeezerVison

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

GeezerVison is showing github.com — GH-98831: Add `macro` and `op` and their implementation to DSL by gvanrossum · Pull Request #99495 · python/cpython

Conversation

gvanrossum commented Nov 14, 2022 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

One deviation from the DSL spec: macro can build on both inst and op.

Also some other random improvements:

Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

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

All reactions

bedevere-bot added the awaiting core review label

Nov 14, 2022

bedevere-bot mentioned this pull request

Nov 14, 2022

Generate the interpreter #98831

Closed

gvanrossum added the skip news label

Nov 14, 2022

gvanrossum force-pushed the macro-ops branch 2 times, most recently from 4cdb25f to f7895ac Compare

November 15, 2022 06:07

gvanrossum mentioned this pull request

Nov 16, 2022

GH-98831: Refactor and fix cases generator #99526

Merged

gvanrossum added 12 commits

November 17, 2022 17:40


          Add macro() and op() DSL features

7fb598a


          Improve code generation for super/macro instructions

edae5af

(But this needs to be cleaned up for maintainability.)


          Move super code generation into a helper

881357e


          Reduce the fiddling with integers in super analysis

0f12c40

Also:
- Add helpers for indent management;
- Reformat with black.


          Do the super/macro analysis at analysis time

babdbf9


          Convert WITH_EXCEPT_START, fix generator to make it work

2fa0a04


          Fix lexer to balk at unrecognized characters, e.g. '@'

cb0c874


          Fix typo in comment

fe0d336


          Code review from pythonGH-99526

d84a5c3


          Fix moved output names; support object pointers in cache

d7ad950


          Tune README

a034675


          Introduce error() method to print errors

1aafac8

gvanrossum force-pushed the macro-ops branch from 598e71a to 1aafac8 Compare

November 18, 2022 02:44

gvanrossum added 2 commits

November 17, 2022 19:01


          Check components of super/macro ops

20062f4


          Fix crash when WITH_EXCEPT_START errors out

This was popping the common items off the stack.

gvanrossum marked this pull request as ready for review

November 18, 2022 06:10

gvanrossum requested a review from brandtbucher

November 18, 2022 06:10

gvanrossum commented

Nov 18, 2022

View reviewed changes

Comment thread

Python/generated_cases.c.h


		TARGET(WITH_EXCEPT_START) {
		PyObject *val = PEEK(1);
		PyObject *lasti = PEEK(3);

gvanrossum Nov 18, 2022

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.

This warning is annoying, the variable is used in an assert() call.

Sorry, something went wrong.

Uh oh!

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

All reactions

brandtbucher Nov 22, 2022

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 stick a (void)lasti; // <helpful comment> near the assert?

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Nov 22, 2022

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.

Oooh, I didn't realize you could do that. Will do.

Sorry, something went wrong.

Uh oh!

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

All reactions


          Merge branch 'main' into macro-ops

cb62653

gvanrossum commented Nov 20, 2022

Copy link

Copy Markdown

Member Author

FWIW I requested a buildbot run for GH-99601, which is 2 commits ahead of this one. If that passes I will assume it would pass here too, without actually trying it (leave some resources for others).

All reactions

Sorry, something went wrong.

Uh oh!

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

brandtbucher approved these changes

Nov 22, 2022

View reviewed changes

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

Thanks for your patience! I think this is a good step forward.

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


		TARGET(WITH_EXCEPT_START) {
		PyObject *val = PEEK(1);
		PyObject *lasti = PEEK(3);

brandtbucher Nov 22, 2022

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 stick a (void)lasti; // <helpful comment> near the assert?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Tools/cases_generator/generate_cases.py Outdated

Comment on lines +88 to +91

		if ceffect.size == 1:
		# There is no read_u16() helper function.
		f.write(f"*(next_instr + {cache_offset});\n")
		else:

brandtbucher Nov 22, 2022

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.

Eh, should we just add a read_u16 function to keep things simple? They're not really special anymore now that we've ditched the structs, and we've shown that it would compile the same.

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Nov 22, 2022

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, good idea. Will do.

Sorry, something went wrong.

Uh oh!

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

All reactions

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 Resolved

Uh oh!

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

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 Resolved

Uh oh!

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

Comment thread

Python/generated_cases.c.h

Comment on lines +3773 to +3774

		PyObject *_tmp_1 = PEEK(2);
		PyObject *_tmp_2 = PEEK(1);

brandtbucher Nov 22, 2022

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.

Just a note on the generated code... this looks sort of odd to me. _tmp_1 is the second item on the stack, and is used by the second instruction part. _tmp_2 is the first item on the stack and is used by the first instruction part.

Not a big deal, but it seems like they should be named in the order they are actually used, like the other superinstructions? My brain took a second to parse and re-parse the generated code a couple of times.

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Nov 22, 2022 •

edited

Loading

Uh oh!

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

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.

I agree this is annoying, but it's hard to get the numbering consistent since there may be variables that aren't popped off the stack but are pushed back onto it. I could number them in reverse order, but there could still be cases where the numbering would be off.

E.g. for stack effect (a, b -- c) we'd see something like

PyObject *_tmp_3 = PEEK(2);
PyObject *_tmp_2 = PEEK(1);
PyObject *_tmp_1;
{
    PyObject *a = _tmp_3;
    PyObject *b = _tmp_2;
    PyObject *c;
    <c = a + b>
    _tmp_1 = c;
}
STACK_GROW(1);
PEEK(1, _tmp_1);

At some point I considered using letters instead of number the temp variables, but that wouldn't help much.

Once we have a register VM it'll be less of an issue. :-)

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Tools/cases_generator/parser.py


		@dataclass
		class InstHeader(Node):
		kind: Literal["inst", "op"]

brandtbucher Nov 22, 2022

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.

The "kind" tag used here and elsewhere seems a bit odd to me. Is it just more ergonomic than using subclassing?

Sorry, something went wrong.

Uh oh!

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

All reactions

gvanrossum Nov 22, 2022

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.

It's historic. I'm slowly getting rid of these.

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

Nov 22, 2022


          Don't use typing.Dict

b74aa6a

I blame Copilot. :-)

Co-authored-by: Brandt Bucher <[email protected]>

gvanrossum and others added 5 commits

November 22, 2022 13:28


          Kill more "unused" literals

a0feff9

I thought I got them all...

Co-authored-by: Brandt Bucher <[email protected]>


          Don't over-use f-strings

a2e9991

Co-authored-by: Brandt Bucher <[email protected]>


          Avoid compiler warning on unused variable

240126b


          Merge remote-tracking branch 'origin/main' into macro-ops


          Introduce read_uint16(p) as equivalent to *p

7afa58c

This simplifies a few lines in the code generator.

gvanrossum commented Nov 22, 2022

Copy link

Copy Markdown

Member Author

I'll just land once tests pass. The read_uint16() function was needed only once. :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum merged commit 8f18ac0 into python:main

Nov 23, 2022

bedevere-bot removed the awaiting merge label

Nov 23, 2022

gvanrossum deleted the macro-ops branch

November 23, 2022 00:05

gvanrossum restored the macro-ops branch

November 23, 2022 04:46

gvanrossum deleted the macro-ops branch

November 23, 2022 05:36

Reviewers

brandtbucher

brandtbucher approved these changes

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 Nov 14, 2022 • edited 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!

gvanrossum commented Nov 20, 2022

Uh oh!

brandtbucher 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gvanrossum Nov 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gvanrossum commented Nov 22, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gvanrossum commented Nov 14, 2022 •

edited

Loading

Uh oh!

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

gvanrossum Nov 22, 2022 •

edited

Loading

Uh oh!

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