GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — GH-98831: Support cache effects in super- and macro instructions by gvanrossum · Pull Request #99601 · python/cpython Conversation
|
|
🤖 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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
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.
594787e to
d5717e3
Compare
|
@brandtbucher If you want larger diffs I can send you a later version that also updates a bunch of instructions ( |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
As before, a later version passed all the buildbot tests and I am confident this one would too. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| if dedent != 0 and tkn.kind == 'COMMENT' and '\n' in text: | ||
| if dedent < 0: | ||
| text = text.replace('\n', '\n' + ' '*-dedent) | ||
| # TODO: dedent > 0 |
There was a problem 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.
There was an error while loading. Please reload this page.
There was a problem 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.
There was an error while loading. Please reload this page.
| 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) |
There was a problem 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.
There was an error while loading. Please reload this page.
| 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(): |
There was a problem 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?
| 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.
There was an error while loading. Please reload this page.
| else: | ||
| return CacheEffect(tkn.text, size) | ||
| raise self.make_syntax_error("Expected integer") | ||
| else: |
There was a problem 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.
There was an error while loading. Please reload this page.
| 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") | ||
| ) |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Sorta defeats the purpose of os.path.join... ;)
| 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.
There was an error while loading. Please reload this page.
| for i, var in enumerate(reversed(up.stack[: up.final_sp]), 1): | ||
| self.out.emit(f"POKE({i}, {var});") | ||
|
|
||
| self.out.emit(f"DISPATCH();") |
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
| self.out.emit(f"DISPATCH();") | |
| self.out.emit("DISPATCH();") |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Yeah, it would be nice if instead of we could write I had originally thought that But I only came up with that while writing this reply, I'll have to play with it to see if it'll work. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.