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: Typed stack effects, and more instructions converted #99764

Merged
merged 34 commits into from
Dec 8, 2022

Conversation

gvanrossum
Copy link
Member

@gvanrossum gvanrossum commented Nov 25, 2022

This started out as just converting a bunch of instructions (a few small families) to the new format. I then ended up adding:

  • Support for typed stack effects (e.g. jump: size_t)
  • Some general refactoring of the super/macro code
  • Reversing the temp variable numbering

@gvanrossum gvanrossum added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 25, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 522dc9a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 25, 2022
@gvanrossum gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 26, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit 1717003 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 26, 2022
@gvanrossum gvanrossum force-pushed the typed-stack-effects branch 2 times, most recently from fdb0dbd to 8f52910 Compare November 28, 2022 00:06
@gvanrossum
Copy link
Member Author

Trying to debug the Windows crashes, my only idea (while I'm traveling without my Windows laptop) is to bisect by hand, pushing each time until I've identified the commit that broke it. It passed in GH-99601 which is a direct ancestor.

@gvanrossum
Copy link
Member Author

The culprit appears to be commit 5e433d6, "LIST_APPEND (a bit unhappy with it)". I'll have to closely read that commit again.

@gvanrossum
Copy link
Member Author

Well d'oh, the generated code has this beauty:

            PREDICT(JUMP_BACKWARD);
            STACK_SHRINK(1);
            DISPATCH();

I need to go back to the drawing board for this. Presumably if the C code block ends with PREDICT() we need to insert the STACK_SHRINK() before that.

@gvanrossum
Copy link
Member Author

So this only failed on Windows because the other popular platforms use computed goto and then PREDICT() is a no-op.

Maybe the answer is faster-cpython/ideas#496 (proposal to get rid of PREDICT()). There are only 10 PREDICT() calls left. We should benchmark this on Windows first though.

@gvanrossum gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit aff6852 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2022
@gvanrossum
Copy link
Member Author

The one failing test seems to be unrelated -- several other builds on that machine are also failing. https://buildbot.python.org/all/#/builders/387

@brandtbucher brandtbucher self-requested a review December 3, 2022 01:04
This doesn't really make things much cleaner, but it works.
This was more convoluted than I expected.
Also removed some dead code from the former
(somehow the case for CacheEffect had crept back in).
@gvanrossum gvanrossum marked this pull request as ready for review December 4, 2022 00:34
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.

Looks mostly good, just a few questions and other things I noticed:

Comment on lines +222 to +225
type = ""
if self.expect(lx.COLON):
type = self.require(lx.IDENTIFIER).text
return StackEffect(tkn.text, type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge fan of shadowing type here:

Suggested change
type = ""
if self.expect(lx.COLON):
type = self.require(lx.IDENTIFIER).text
return StackEffect(tkn.text, type)
type_ = ""
if self.expect(lx.COLON):
type_ = self.require(lx.IDENTIFIER).text
return StackEffect(tkn.text, type_)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of suffix _, but I hear you, and I can change this to typ. I'll make a pass.

goto error;
PyObject *v = PEEK(1);
PyObject *list = PEEK(oparg + 1); // +1 to account for v staying on stack
if (_PyList_AppendTakeRef((PyListObject *)list, v) < 0) goto pop_1_error;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any plans to make these conform to PEP 7 (braces and newline for body)?

I know this isn't exactly meant for human consumption, but as a human who will definitely be consuming this output, I think it might be worth improving the readability of the generated code a bit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the issue that you easily miss the goto, or is your brain just yelling "missing braces" at you? I intentionally put it all on one line so the reason why PEP 7 requires braces doesn't apply. I think there's also a technical issue where we suddenly need to know the indentation level (but that's recovered easily enough).

All in all my preferred answer is "no". :-) But I feel you. My brain yells at me a little bit too.

if (hook == NULL) {
_PyErr_SetString(tstate, PyExc_RuntimeError,
"lost sys.displayhook");
Py_DECREF(value);
goto error;
ERROR_IF(1, error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This took me a second to parse. Maybe it's worth adding another ERROR(LABEL) macro for unconditional errors?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with ERROR(error) is that the generator would need to intercept that too (since possibly it needs to go to _pop_1_error), and the intercept code is slightly hairy already. Maybe ERROR_IF(true, error) works? I'll try that.

goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_STORE_SUBSCR);
ERROR_IF(err != 0, error);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: the != 0 is just adding noise here (I had to double-check the condition when reading). Maybe just:

Suggested change
ERROR_IF(err != 0, error);
ERROR_IF(err, error);

(Elsewhere too.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, the original had it too. Agreed it's redundant, my brain doesn't see it as noise though (the original never gave me a second thought).

@@ -81,8 +81,19 @@ do { \
// Dummy variables for stack effects.
static PyObject *value, *value1, *value2, *left, *right, *res, *sum, *prod, *sub;
static PyObject *container, *start, *stop, *v, *lhs, *rhs;
static PyObject *list, *tuple, *dict;
static PyObject *exit_func, *lasti, *val;
static PyObject *list, *tuple, *dict, *owner;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to have the generator also build a header with all of these lines? You would need to run the generator at least once before the red squiggles go away, but then we don't have to worry about manually maintaining this blob of stuff.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this part of the tooling is kind of annoying. But having the generator also update bytecodes.c seems sneaky (and slightly dangerous).

Comment on lines +1998 to +2000
_COMPARE_OP_FLOAT,
_COMPARE_OP_INT,
_COMPARE_OP_STR,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, so families can't contain superinstructions? I'm not sure I would have been able to figure out this trick on my own... :(

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see faster-cpython/ideas#495 (comment). This is definitely suboptimal.

jump = sign_ish & when_to_jump_mask;
}
// The input is an int disguised as an object pointer!
op(_JUMP_ON_SIGN, (jump: size_t --)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call this _JUMP_IF? It's more general than on "sign" alone, and might be useful elsewhere...

Suggested change
op(_JUMP_ON_SIGN, (jump: size_t --)) {
op(_JUMP_IF, (jump: uintptr_t --)) {

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, you're right, that uop needs a better name. I like _JUMP_IF. Will do.

STACK_SHRINK(3);
next_instr += 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be more readable to spell all these as JUMPBY(...)?

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. Although there will still be explicit mentions of next_instr in the read_uNN() calls.

Comment on lines 3629 to 3630
PyObject *_tmp_2 = PEEK(2);
PyObject *_tmp_1 = 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.

Minor: these got better, but still seem "backwards". Not sure if you noticed.

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, you'd want the PEEK(1) to come before the PEEK(2). I could do that.

def declare(self, dst: StackEffect, src: StackEffect | None):
if dst.name == UNUSED:
return
type = f"{dst.type} " if dst.type else "PyObject *"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we just set the default value of type to "PyObject *" in the parser to avoid this move here and elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd be generating slightly un-idiomatic code where there's a space after the star. I'm not sure what bugs me more. I think the code I already wrote wins though. :-)

Copy link
Member Author

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for doing one-by-one responses to the first few comments. If I do the things you've asked for, do you want another chance at review?

Comment on lines 277 to 280
// Part 1's output effect is a lie -- it has no result.
// Part 2's input effect is equally a lie, and the two lies
// cancel each other out.
op(_BINARY_OP_INPLACE_ADD_UNICODE_PART_1, (left, right, unused/1 -- unused)) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lies are so that we can add _BINARY_OP_INPLACE_ADD_UNICODE_PART_1 to the family and define the thing as a proper super-instruction.

Since you react rather strongly to this I think maybe I should just roll that back. See also faster-cpython/ideas#495 (comment).

PyObject *list = PEEK(oparg);
if (_PyList_AppendTakeRef((PyListObject *)list, v) < 0)
goto error;
// Alternative: (list, unused[oparg], v -- list, unused[oparg])
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but those are still only a faint glimmer on the horizon. Duly noted though.

Python/bytecodes.c Show resolved Hide resolved
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_STORE_SUBSCR);
ERROR_IF(err != 0, error);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, the original had it too. Agreed it's redundant, my brain doesn't see it as noise though (the original never gave me a second thought).

assert(frame == &entry_frame);
assert(_PyFrame_IsIncomplete(frame));
PyObject *retval = POP();
STACK_SHRINK(1); // Since we're not going to DISPATCH()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without it the assert(EMPTY()) on the next line fails. I'm kind of hoping that when asserts are off, the compiler sees that the stack pointer is dead at this point and won't bother to generate code for it. Then again, the original had POP() which also adjusts the stack pointer.

Comment on lines +1998 to +2000
_COMPARE_OP_FLOAT,
_COMPARE_OP_INT,
_COMPARE_OP_STR,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also see faster-cpython/ideas#495 (comment). This is definitely suboptimal.

jump = sign_ish & when_to_jump_mask;
}
// The input is an int disguised as an object pointer!
op(_JUMP_ON_SIGN, (jump: size_t --)) {
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, you're right, that uop needs a better name. I like _JUMP_IF. Will do.

STACK_SHRINK(3);
next_instr += 1;
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. Although there will still be explicit mentions of next_instr in the read_uNN() calls.

Comment on lines 3629 to 3630
PyObject *_tmp_2 = PEEK(2);
PyObject *_tmp_1 = PEEK(1);
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, you'd want the PEEK(1) to come before the PEEK(2). I could do that.

def declare(self, dst: StackEffect, src: StackEffect | None):
if dst.name == UNUSED:
return
type = f"{dst.type} " if dst.type else "PyObject *"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then we'd be generating slightly un-idiomatic code where there's a space after the star. I'm not sure what bugs me more. I think the code I already wrote wins though. :-)

@gvanrossum
Copy link
Member Author

gvanrossum commented Dec 8, 2022

TODO from review:

  • Generate PEEKs and POKEs in reverse order
  • Use JUMPBY(n) instead of next_instr += n
  • Change type to typ in generate_cases.py
  • Rename _JUMP_ON_SIGN to _JUMP_IF
  • Move all cache effects to the front of the effects
  • ERROR_IF(err != 0, error) -> ERROR_IF(err, error)
  • Roll back BINARY_OP_INPLACE_ADD_UNICODE
  • ERROR_IF(1, error) -> ERROR_IF(true, error) [assuming true exists]

Not doing (maybe later):

  • (Later?) Avoid writing *cache = ..., roll this into DECREMENT_ADAPTIVE_COUNTER()
  • Add braces around generated gotos? What about newlines?

@netlify
Copy link

netlify bot commented Dec 8, 2022

Deploy Preview for python-cpython-preview ready!

Name Link
🔨 Latest commit c6dfeec
🔍 Latest deploy log https://app.netlify.com/sites/python-cpython-preview/deploys/639209a248c88400083c9c50
😎 Deploy Preview https://deploy-preview-99764--python-cpython-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member Author

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to do the following:

  • wait for the tests to pass
  • merge main
  • request buildbots
  • wait for buildbot tests to pass
  • merge it

It'll probably take all mornning. Stop me if you want me to change anything; if I don't hear from you I'll just go ahead with merging.

@gvanrossum gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 8, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @gvanrossum for commit c6dfeec 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 8, 2022
@gvanrossum
Copy link
Member Author

It looks like the ARM64 buildbot failure is not this PR's fault, its other runs also failed in the last day or so.

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 the changes!

@gvanrossum gvanrossum merged commit c85be73 into python:main Dec 8, 2022
@gvanrossum gvanrossum deleted the typed-stack-effects branch December 8, 2022 23:45
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