Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH-98831: Add macro and op and their implementation to DSL #99495

Merged
merged 21 commits into from
Nov 23, 2022

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Nov 14, 2022

@gvanrossum gvanrossum marked this pull request as ready for review November 18, 2022 06:10
@@ -2754,31 +2745,31 @@
}

TARGET(WITH_EXCEPT_START) {
PyObject *val = PEEK(1);
PyObject *lasti = PEEK(3);
Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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.

@gvanrossum
Copy link
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).

Copy link
Member

@brandtbucher brandtbucher left a 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.

@@ -2754,31 +2745,31 @@
}

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

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?

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:
Copy link
Member

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.

Copy link
Member Author

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.

Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved
Comment on lines +3773 to +3774
PyObject *_tmp_1 = PEEK(2);
PyObject *_tmp_2 = PEEK(1);
Copy link
Member

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.

Copy link
Member Author

@gvanrossum gvanrossum Nov 22, 2022

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

@@ -74,16 +74,22 @@ class CacheEffect(Node):

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

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?

Copy link
Member Author

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.

I blame Copilot. :-)

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
gvanrossum and others added 5 commits November 22, 2022 13:28
I thought I got them all...

Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
Co-authored-by: Brandt Bucher <brandtbucher@gmail.com>
This simplifies a few lines in the code generator.
@gvanrossum
Copy link
Member Author

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

@gvanrossum gvanrossum merged commit 8f18ac0 into python:main Nov 23, 2022
@gvanrossum gvanrossum deleted the macro-ops branch November 23, 2022 00:05
@gvanrossum gvanrossum restored the macro-ops branch November 23, 2022 04:46
@gvanrossum gvanrossum deleted the macro-ops branch November 23, 2022 05:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants