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 Nov 18, 2022
@gvanrossum gvanrossum requested a review from brandtbucher Nov 18, 2022
@@ -2754,31 +2745,31 @@
}

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

@gvanrossum gvanrossum Nov 18, 2022

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

@brandtbucher brandtbucher Nov 22, 2022

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

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

Oooh, I didn't realize you could do that. Will do.

@gvanrossum
Copy link
Member Author

gvanrossum commented Nov 20, 2022

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

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

@brandtbucher brandtbucher Nov 22, 2022

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?

if ceffect.size == 1:
# There is no read_u16() helper function.
f.write(f"*(next_instr + {cache_offset});\n")
else:
Copy link
Member

@brandtbucher brandtbucher Nov 22, 2022

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

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

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
PyObject *_tmp_1 = PEEK(2);
PyObject *_tmp_2 = PEEK(1);
Copy link
Member

@brandtbucher brandtbucher Nov 22, 2022

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

@brandtbucher brandtbucher Nov 22, 2022

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

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

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 Nov 22, 2022
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

gvanrossum commented Nov 22, 2022

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
13 checks passed
@gvanrossum gvanrossum deleted the macro-ops branch Nov 23, 2022
@gvanrossum gvanrossum restored the macro-ops branch Nov 23, 2022
@gvanrossum gvanrossum deleted the macro-ops branch Nov 23, 2022
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.

None yet

3 participants