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

gvanrossum · 2022-11-19T18:30:34Z

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

bedevere-bot · 2022-11-19T22:05:48Z

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

gvanrossum · 2022-11-25T05:48:38Z

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

gvanrossum · 2022-11-25T20:43:35Z

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

gvanrossum

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

Tools/cases_generator/parser.py

Python/generated_cases.c.h

Tools/cases_generator/generate_cases.py

brandtbucher

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.

brandtbucher · 2022-12-03T00:36:51Z

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


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

brandtbucher · 2022-12-03T00:41:49Z

Tools/cases_generator/parser.py

            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)


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

brandtbucher · 2022-12-03T00:46:05Z

Tools/cases_generator/parser.py

+        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():


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.

brandtbucher · 2022-12-03T00:49:41Z

Tools/cases_generator/parser.py

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


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.

brandtbucher · 2022-12-03T00:52:33Z

Tools/cases_generator/generate_cases.py

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


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

)

brandtbucher · 2022-12-03T01:01:49Z

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


Suggested change

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

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

gvanrossum · 2022-12-03T03:46:34Z

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gvanrossum commented Nov 19, 2022 •

edited

Loading

bedevere-bot commented Nov 19, 2022

gvanrossum commented Nov 25, 2022

gvanrossum commented Nov 25, 2022

gvanrossum left a comment

brandtbucher left a comment

brandtbucher Dec 3, 2022

gvanrossum Dec 3, 2022

brandtbucher Dec 3, 2022

brandtbucher Dec 3, 2022

brandtbucher Dec 3, 2022

brandtbucher Dec 3, 2022

brandtbucher Dec 3, 2022

gvanrossum commented Dec 3, 2022

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

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

Conversation

gvanrossum commented Nov 19, 2022 • edited Loading

Details

Super-instructions can now have cache effects.

Macro instructions can now also have cache effects.

NOTES

bedevere-bot commented Nov 19, 2022

gvanrossum commented Nov 25, 2022

gvanrossum commented Nov 25, 2022

gvanrossum left a comment

Choose a reason for hiding this comment

brandtbucher left a comment

Choose a reason for hiding this comment

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

gvanrossum Dec 3, 2022

Choose a reason for hiding this comment

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

brandtbucher Dec 3, 2022

Choose a reason for hiding this comment

gvanrossum commented Dec 3, 2022

gvanrossum commented Nov 19, 2022 •

edited

Loading