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: Support cache effects in super- and macro instructions by gvanrossum · Pull Request #99601 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Nov 19, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Note: this doesn't yet support cache effects in the macro definition itself (e.g. macro(X) = counter/1 + FOO + BAR + unused/2). That seems something for a follow-up (we don't even have a use case for the current thing yet).

Details

(From two original commits that were since merged.)

Super-instructions can now have cache effects.

To do this I mostly just had to move the cache effects code into
Instr*.write_body(), reducing the responsibilities of Instr*.write().
(I also had to fiddle a bit with indents.)

For macros the same approach would almost work, except that
next_instr might point in the middle of the cache when we encounter
DEOPT_IF() or ERROR_IF() in a second or further component.
I have to think more about that.

NOTES:

  • There is no test example yet (I manually tested it with a fake new
    super-instruction).
  • The cache-related generated code is in different place.
    This shouldn't matter.

Macro instructions can now also have cache effects.

We pass the initial cache offset into write_body().

This is a little fiddly because everything is different
for super-instructions vs macros:

  • For super, cache_adjust is always zero because we
    bump next_instr after each op.
  • For macro, cache_adjust accumulates previous cache offsets,
    and we bump next_instr at the end.

Also, I had to move the bump of next_instr back into Instr*.write().
It is better placed there anyway because that function avoids the bump
if the C code already ends in a goto, return or DISPATCH*() call.
(The previous commit emitted one unreachable bump, which is now fixed.)

Tested manually.

NOTES

There's more refactoring coming.

Also included:

  • Fix dedent of comments in to_text()
  • Flatten InstDef
  • Improve CLI help output
  • Other refactoring (quite a bit, sorry)

  • 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 bedevere-bot mentioned this pull request Nov 19, 2022
Generate the interpreter #98831
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 19, 2022
@gvanrossum gvanrossum added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 19, 2022
@bedevere-bot

bedevere-bot commented Nov 19, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit b92e879 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2022
@gvanrossum gvanrossum mentioned this pull request Nov 20, 2022
GH-98831: Add macro and op and their implementation to DSL #99495
Merged
gvanrossum added 7 commits November 22, 2022 21:22
@gvanrossum
Fix dedent of comments in to_text()
e6d8fdf
@gvanrossum
Implement cache effects for super- and macro instruction
2731077
This once again refactors a lot of the code generator.
There are still cleanups to be done, and I'd like things to be
more compact, but I think most of the refactoring is now done.
@gvanrossum
Regenerated cases
5c97e3f
@gvanrossum
Consistently rename sup(er), mac(ro)
a3af889
sup: SuperInstruction
mac: MacroInstruction
super: Super
macro: Macro
@gvanrossum
Flatten InstDef; improve CLI help output
4687fba
I tried to split it into InstDef and OpDef, removing kind,
but that caused problems because the Instruction class
inherits from InstHeader.
@gvanrossum
Tweak the structure of Instruction and {Super,Macro}Instruction
59e63d4
@gvanrossum
Refactor write_{super,macro} to share more code
d5717e3
@gvanrossum
gvanrossum force-pushed the macro-cache-effects branch from 594787e to d5717e3 Compare November 25, 2022 05:37
@gvanrossum
gvanrossum requested a review from brandtbucher November 25, 2022 05:44
@gvanrossum
gvanrossum marked this pull request as ready for review November 25, 2022 05:44
@gvanrossum

gvanrossum commented Nov 25, 2022

Copy link
Copy Markdown
Member Author

@brandtbucher If you want larger diffs I can send you a later version that also updates a bunch of instructions (BINARY_OP_INPLACE_ADD_UNICODE and COMPARE_OP*) and adds typed stack effects.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum mentioned this pull request Nov 25, 2022
GH-98831: Typed stack effects, and more instructions converted #99764
Merged
@gvanrossum

gvanrossum commented Nov 25, 2022

Copy link
Copy Markdown
Member Author

As before, a later version passed all the buildbot tests and I am confident this one would too.

All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum
gvanrossum commented Nov 29, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

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.

Here are a few hints about the refactorings.

This PR intentionally doesn't contain any changes to instruction definitions, to highlight that the generated output is unchanged (except for one detail).

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 Resolved

Uh oh!

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

Comment thread Python/generated_cases.c.h 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 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 Tools/cases_generator/generate_cases.py Resolved

Uh oh!

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

brandtbucher
brandtbucher approved these changes Dec 3, 2022
View reviewed changes

@brandtbucher 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! Excited to see these in action. :)

Do we have any plans to desugar superinstructions into macros? Could help unify some of the repeated logic in the generator.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Tools/cases_generator/lexer.py
if dedent != 0 and tkn.kind == 'COMMENT' and '\n' in text:
if dedent < 0:
text = text.replace('\n', '\n' + ' '*-dedent)
# TODO: dedent > 0

@brandtbucher brandtbucher Dec 3, 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.

Leaving this TODO for future work?

As a minor nit: the double-negative of a "negative dedent" is sort of strange to me. I'd personally find it easier to reason about "indents" and "negative indents"... but I'm not sure if that makes things more difficult elsewhere.

Sorry, something went wrong.

Uh oh!

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

All reactions

@gvanrossum gvanrossum Dec 3, 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.

Yeah, I inherited that from the lexer. I'll eventually fix it. For a while it was inconvenient because there was also a variable named indent in a lot of places. That's now self.indent. (But still, it's a string of spaces rather than an int.)

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
Comment on lines 235 to +237
while self.expect(lx.PLUS):
if tkn := self.require(lx.IDENTIFIER):
ops.append(tkn.text)
self.require(lx.SEMI)
if op := self.op():
ops.append(op)

@brandtbucher brandtbucher Dec 3, 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.

Does this allow a single op followed by a bunch of +s? It looks like it might...

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
Comment on lines +247 to +252
if (tkn := self.expect(lx.IDENTIFIER)) and tkn.text == "macro":
if self.expect(lx.LPAREN):
if tkn := self.expect(lx.IDENTIFIER):
if self.expect(lx.RPAREN):
if self.expect(lx.EQUALS):
if uops := self.uops():

@brandtbucher brandtbucher Dec 3, 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.

Hmmmm. I know you prefer one test per if, but I think six nested ifs is pushing it for generated code, let alone human code. Maybe one test per line?

Suggested change
if (tkn := self.expect(lx.IDENTIFIER)) and tkn.text == "macro":
if self.expect(lx.LPAREN):
if tkn := self.expect(lx.IDENTIFIER):
if self.expect(lx.RPAREN):
if self.expect(lx.EQUALS):
if uops := self.uops():
if (
(tkn := self.expect(lx.IDENTIFIER))
and tkn.text == "macro"
and self.expect(lx.LPAREN)
and (tkn := self.expect(lx.IDENTIFIER))
and self.expect(lx.RPAREN)
and self.expect(lx.EQUALS)
and (uops := self.uops())
):

But consistency within the file is probably more important.

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
else:
return CacheEffect(tkn.text, size)
raise self.make_syntax_error("Expected integer")
else:

@brandtbucher brandtbucher Dec 3, 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.

This is another case where the deep nesting impairs readability: with the naked eye, it's pretty hard to tell which if this else corresponds to.

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
Comment on lines +17 to +22
DEFAULT_INPUT = os.path.relpath(
os.path.join(os.path.dirname(__file__), "../../Python/bytecodes.c")
)
DEFAULT_OUTPUT = os.path.relpath(
os.path.join(os.path.dirname(__file__), "../../Python/generated_cases.c.h")
)

@brandtbucher brandtbucher Dec 3, 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.

Sorta defeats the purpose of os.path.join... ;)

Suggested change
DEFAULT_INPUT = os.path.relpath(
os.path.join(os.path.dirname(__file__), "../../Python/bytecodes.c")
)
DEFAULT_OUTPUT = os.path.relpath(
os.path.join(os.path.dirname(__file__), "../../Python/generated_cases.c.h")
)
DEFAULT_INPUT = os.path.relpath(
os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, "Python", "bytecodes.c")
)
DEFAULT_OUTPUT = os.path.relpath(
os.path.join(os.path.dirname(__file__), os.pardir, os.pardir, "Python", "generated_cases.c.h")
)

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
for i, var in enumerate(reversed(up.stack[: up.final_sp]), 1):
self.out.emit(f"POKE({i}, {var});")

self.out.emit(f"DISPATCH();")

@brandtbucher brandtbucher Dec 3, 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.

Suggested change
self.out.emit(f"DISPATCH();")
self.out.emit("DISPATCH();")

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Dec 3, 2022
@gvanrossum

gvanrossum commented Dec 3, 2022

Copy link
Copy Markdown
Member Author

Do we have any plans to desugar superinstructions into macros? Could help unify some of the repeated logic in the generator.

Yeah, it would be nice if instead of

super(LOAD_FAST__LOAD_CONST) = LOAD_FAST + LOAD_CONST;

we could write

macro(LOAD_FAST__LOAD_CONST) = LOAD_FAST + JOIN + LOAD_CONST;

I had originally thought that JOIN could be defined like this:

op(JOIN, (--)) {
    NEXTOPARG();
    next_instr++;
}

But next_instr isn't pointing where we expect it to be pointing, and bumping it will make things worse. Instead we could define

op(JOIN, (word/1 --)) {
    oparg = _Py_OPARG(word);
}

I only came up with that while writing this reply, I'll have to play with it to see if it'll work.

All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum
gvanrossum merged commit acf9184 into python:main Dec 3, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 3, 2022
@gvanrossum
gvanrossum deleted the macro-cache-effects branch December 3, 2022 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@gvanrossum @bedevere-bot @brandtbucher
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.