GH-98831: Support cache effects in super- and macro instructions by gvanrossum · Pull Request #99601 · python/cpython · GitHub

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

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

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.

sup: SuperInstruction mac: MacroInstruction super: Super macro: Macro

I tried to split it into InstDef and OpDef, removing kind, but that caused problems because the Instruction class inherits from InstHeader.

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

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

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

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.

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.

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

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

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.

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.

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")
          
            )

Suggested change

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

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.

bedevere-bot mentioned this pull request Nov 19, 2022

Generate the interpreter #98831

Closed

bedevere-bot added the awaiting core review label Nov 19, 2022

gvanrossum added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 19, 2022

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2022

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

Fix dedent of comments in to_text()

e6d8fdf

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.

Regenerated cases

5c97e3f

Consistently rename sup(er), mac(ro)

a3af889

sup: SuperInstruction mac: MacroInstruction super: Super macro: Macro

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.

Tweak the structure of Instruction and {Super,Macro}Instruction

59e63d4

Refactor write_{super,macro} to share more code

d5717e3

gvanrossum force-pushed the macro-cache-effects branch from 594787e to d5717e3 Compare November 25, 2022 05:37

gvanrossum requested a review from brandtbucher November 25, 2022 05:44

gvanrossum marked this pull request as ready for review November 25, 2022 05:44

gvanrossum mentioned this pull request Nov 25, 2022

GH-98831: Typed stack effects, and more instructions converted #99764

Merged

gvanrossum commented Nov 29, 2022

View reviewed changes

brandtbucher approved these changes Dec 3, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Dec 3, 2022

gvanrossum merged commit acf9184 into python:main Dec 3, 2022

bedevere-bot removed the awaiting merge label Dec 3, 2022

gvanrossum deleted the macro-cache-effects branch December 3, 2022 05:25

+                      if dedent != 0 and tkn.kind == 'COMMENT' and '\n' in text:
+                          if dedent < 0:
+                              text = text.replace('\n', '\n' + ' '*-dedent)
+                          # TODO: dedent > 0

                           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)

+                                  else:
+                                      return CacheEffect(tkn.text, size)
+                              raise self.make_syntax_error("Expected integer")
+                          else:

+                          for i, var in enumerate(reversed(up.stack[: up.final_sp]), 1):
+                              self.out.emit(f"POKE({i}, {var});")
+                          self.out.emit(f"DISPATCH();")

GH-98831: Support cache effects in super- and macro instructions by gvanrossum · Pull Request #99601 · python/cpython · GitHub

Uh oh!

Uh oh!

GH-98831: Support cache effects in super- and macro instructions - #99601

GH-98831: Support cache effects in super- and macro instructions#99601gvanrossum merged 7 commits intopython:mainpython/cpython:mainfrom gvanrossum:macro-cache-effectsgvanrossum/cpython:macro-cache-effectsCopy head branch name to clipboard

Conversation

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

Uh oh!

Details

Super-instructions can now have cache effects.

Macro instructions can now also have cache effects.

NOTES

Uh oh!

bedevere-bot commented Nov 19, 2022

Uh oh!

gvanrossum commented Nov 25, 2022

Uh oh!

gvanrossum commented Nov 25, 2022

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brandtbucher left a comment

Choose a reason for hiding this comment

Uh oh!

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

gvanrossum Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

gvanrossum commented Dec 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

GH-98831: Support cache effects in super- and macro instructions#99601
gvanrossum merged 7 commits into
python:mainfrom
gvanrossum:macro-cache-effects

gvanrossum commented Nov 19, 2022 •
edited
Loading