GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-98831: Typed stack effects, and more instructions converted by gvanrossum · Pull Request #99764 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Nov 25, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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
  • Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot mentioned this pull request Nov 25, 2022
Generate the interpreter #98831
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 25, 2022
@gvanrossum gvanrossum added skip news 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Nov 25, 2022
@bedevere-bot

bedevere-bot commented Nov 25, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

bedevere-bot commented Nov 26, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

gvanrossum commented Nov 28, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum force-pushed the typed-stack-effects branch from 141bdb9 to d24cc5a Compare November 28, 2022 00:44
@gvanrossum

gvanrossum commented Nov 28, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Nov 28, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum

gvanrossum commented Nov 28, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
gvanrossum force-pushed the typed-stack-effects branch from fa0df3e to aff6852 Compare November 28, 2022 06:48
@gvanrossum gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2022
@bedevere-bot

bedevere-bot commented Nov 28, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 28, 2022
@gvanrossum gvanrossum mentioned this pull request Nov 29, 2022
GH-98831: Support cache effects in super- and macro instructions #99601
Merged
@gvanrossum

gvanrossum commented Dec 2, 2022

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brandtbucher
brandtbucher self-requested a review December 3, 2022 01:04
gvanrossum added 7 commits December 2, 2022 21:24
@gvanrossum
Make BINARY_OP_INPLACE_ADD_UNICODE a legit super instruction
6860ed7 
This doesn't really make things much cleaner, but it works.
@gvanrossum
COMPARE_OP
dcba32e
@gvanrossum
COMPARE_OP_FLOAT_JUMP
30cb8cd
@gvanrossum
COMPARE_OP_INT_JUMP
6af0a5d
@gvanrossum
COMPARE_OP_STR_JUMP
039efbf
@gvanrossum
Support typed stack effects
2fd8822 
This was more convoluted than I expected.
@gvanrossum
Refactor common code of analyze_{super,macro}
14bbd50 
Also removed some dead code from the former
(somehow the case for CacheEffect had crept back in).
gvanrossum added 3 commits December 2, 2022 21:24
@gvanrossum
Emit PREDICT() macros right before DISPATCH()
f7a54d7 
This should fix the build crash on Windows.
@gvanrossum
LIST_APPEND (a bit unhappy with it)
6ae9e6e
@gvanrossum
SET_ADD (also a bit unhappy with it)
34aa393
@gvanrossum
gvanrossum force-pushed the typed-stack-effects branch from aff6852 to 34aa393 Compare December 3, 2022 06:12
@gvanrossum
gvanrossum marked this pull request as ready for review December 4, 2022 00:34
brandtbucher
brandtbucher approved these changes Dec 8, 2022
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/parser.py
Comment on lines +222 to +225
type = ""
if self.expect(lx.COLON):
type = self.require(lx.IDENTIFIER).text
return StackEffect(tkn.text, type)

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/generated_cases.c.h
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;

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c Outdated
"lost sys.displayhook");
Py_DECREF(value);
goto error;
ERROR_IF(1, error);

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c Outdated
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_STORE_SUBSCR);
ERROR_IF(err != 0, error);

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
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;

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
Comment on lines +1998 to +2000
_COMPARE_OP_FLOAT,
_COMPARE_OP_INT,
_COMPARE_OP_STR,

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c Outdated
jump = sign_ish & when_to_jump_mask;
}
// The input is an int disguised as an object pointer!
op(_JUMP_ON_SIGN, (jump: size_t --)) {

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/generated_cases.c.h Outdated
if (err != 0) goto pop_3_error;
JUMPBY(INLINE_CACHE_ENTRIES_STORE_SUBSCR);
STACK_SHRINK(3);
next_instr += 1;

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/generated_cases.c.h Outdated
Comment on lines +3629 to +3630
PyObject *_tmp_2 = PEEK(2);
PyObject *_tmp_1 = PEEK(1);

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/generate_cases.py Outdated
def declare(self, dst: StackEffect, src: StackEffect | None):
if dst.name == UNUSED:
return
type = f"{dst.type} " if dst.type else "PyObject *"

@brandtbucher brandtbucher Dec 8, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Dec 8, 2022
gvanrossum
gvanrossum commented Dec 8, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member Author

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c Outdated
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)) {

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
PyObject *list = PEEK(oparg);
if (_PyList_AppendTakeRef((PyListObject *)list, v) < 0)
goto error;
// Alternative: (list, unused[oparg], v -- list, unused[oparg])

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Python/bytecodes.c Outdated
goto error;
}
JUMPBY(INLINE_CACHE_ENTRIES_STORE_SUBSCR);
ERROR_IF(err != 0, error);

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
assert(frame == &entry_frame);
assert(_PyFrame_IsIncomplete(frame));
PyObject *retval = POP();
STACK_SHRINK(1); // Since we're not going to DISPATCH()

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c
Comment on lines +1998 to +2000
_COMPARE_OP_FLOAT,
_COMPARE_OP_INT,
_COMPARE_OP_STR,

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/bytecodes.c Outdated
jump = sign_ish & when_to_jump_mask;
}
// The input is an int disguised as an object pointer!
op(_JUMP_ON_SIGN, (jump: size_t --)) {

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/generated_cases.c.h Outdated
if (err != 0) goto pop_3_error;
JUMPBY(INLINE_CACHE_ENTRIES_STORE_SUBSCR);
STACK_SHRINK(3);
next_instr += 1;

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/generated_cases.c.h Outdated
Comment on lines +3629 to +3630
PyObject *_tmp_2 = PEEK(2);
PyObject *_tmp_1 = PEEK(1);

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Tools/cases_generator/generate_cases.py Outdated
def declare(self, dst: StackEffect, src: StackEffect | None):
if dst.name == UNUSED:
return
type = f"{dst.type} " if dst.type else "PyObject *"

@gvanrossum gvanrossum Dec 8, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gvanrossum

gvanrossum commented Dec 8, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum added 4 commits December 7, 2022 21:50
@gvanrossum
Move all cache effects to the front of the effects list
e144db3
@gvanrossum
Rename _JUMP_ON_SIGN to _JUMP_IF
029bf07
@gvanrossum
Change ERROR_IF(err != 0, ...) to ERROR_IF(err, ...)
fa3135f
@gvanrossum
Change ERROR_IF(1, ...) to ERROR_IF(true, ...)
445f20b
@netlify

netlify Bot commented Dec 8, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

✅ 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum added 5 commits December 7, 2022 22:07
@gvanrossum
Revert "Make BINARY_OP_INPLACE_ADD_UNICODE a legit super instruction"
32b4ac0 
It was too fancy, and I have another idea for super instructions
and effects anyways.

This reverts commit 6860ed7.
@gvanrossum
Emit PEEKs in super/macro instructions in correct order
0ef4da7
@gvanrossum
Use JUMPBY(n) instead of next_instr += n
7d19466
@gvanrossum
Rename local variables type to typ
99e8ec6
@gvanrossum
Remove unused op names
c08dbdb
gvanrossum
gvanrossum commented Dec 8, 2022
View reviewed changes

@gvanrossum gvanrossum left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gvanrossum
Merge branch 'main' into typed-stack-effects
c6dfeec
@gvanrossum gvanrossum added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 8, 2022
@bedevere-bot

bedevere-bot commented Dec 8, 2022

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Dec 8, 2022
@gvanrossum

gvanrossum commented Dec 8, 2022

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher
brandtbucher approved these changes Dec 8, 2022
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gvanrossum
gvanrossum merged commit c85be73 into python:main Dec 8, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label 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

Reviewers

@brandtbucher brandtbucher brandtbucher approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@gvanrossum @bedevere-bot @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.