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: Support conditional effects; use for LOAD_ATTR by gvanrossum · Pull Request #101333 · python/cpython

Conversation

@gvanrossum

@gvanrossum gvanrossum commented Jan 26, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

We can now write

inst(OP, (foo if (oparg & 1) -- bar if (oparg & 2)) {
    ...
}

which pops foo off the stack if oparg & 1, otherwise setting it to NULL, and pushes bar onto the stack if oparg & 2 (otherwise ignoring it).

This syntax cannot be combined with an array size on the same effect, but it can be combined with a type (untested). In addition, it is incompatible with an array size closer to the stack top, e.g. foo if (oparg&1), bar[oparg>>1] is not supported (I don't think it's used, and generating code for it would be slightly awkward, but could be done if needed).

The implementation switches to using POP() and PUSH() (from PEEK() and POKE()) for those variables that are either conditional or whose position relative to the bottom (!) of the stack depends on a condition. (See the added test for details.)

To demonstrate this, I converted LOAD_ATTR (but not its specializations).

  • Issue: Generate the interpreter #98831

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gvanrossum added 5 commits January 25, 2023 16:40
@gvanrossum
Add parser support for conditional stack effects
af97a7e 
(Semantics aren't there yet, and the test fails.)
@gvanrossum
Fix a typo in test_generator.py that made it into main :-(
fbcdde3
@gvanrossum
Make conditional effects work, in theory
27246e9
@gvanrossum
Convert LOAD_ATTR to new format
667630e 
First use of conditional stack effects.
@gvanrossum
Conditionalize effect_size()
69dbc5e
@bedevere-bot bedevere-bot added the awaiting core review label Jan 26, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jan 26, 2023
Generate the interpreter #98831
Closed
@gvanrossum
gvanrossum marked this pull request as ready for review January 26, 2023 00:47
@gvanrossum
gvanrossum requested review from brandtbucher and iritkatriel January 26, 2023 00:48
@gvanrossum gvanrossum added the skip news label Jan 26, 2023
gvanrossum
gvanrossum commented Jan 26, 2023
View reviewed changes
Comment thread Python/bytecodes.c Outdated

// error: LOAD_ATTR has irregular stack effect
inst(LOAD_ATTR) {
inst(LOAD_ATTR, (unused/9, owner -- res, res2 if (oparg & 1))) {

@gvanrossum gvanrossum Jan 26, 2023

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.

Check out my comment in the generated code about 'res2'.

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
PREDICTED(LOAD_ATTR);
PyObject *owner = PEEK(1);
PyObject *res;
PyObject *res2;

@gvanrossum gvanrossum Jan 26, 2023

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.

Should this initialize res2 = NULL? Right now I get compiler warnings (in GitHub) about a possibly uninitialized variable:

‘res2’ may be used uninitialized in this function [-Wmaybe-uninitialized]

presumably from the PUSH() on L1798.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Jan 28, 2023

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.

I think the issue is that the compiler can't see that if (oparg & 1) is the same as if (_cond_res2), so it can't tell that res2 is always initialised when it is used. Not sure what to do about it, add a syntax in the DSL to access the condition from within the bytecode definition?

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Jan 28, 2023

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'll just initialize it to NULL. Perhaps some later phase of optimization will recognize that for the dead value it is.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Jan 28, 2023

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.

I think if we rename _cond_res2 to res2_exists and then in the body of the bytecode we do if(res2_exists) /* use res2 */ instead of if(oparg &1) /* use res2 */ then it would make the code clearer. It's also safer, because if you change the args to the instr then oparg&1 might not be correct anymore, but res2_exists will be automatically updated.

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Jan 28, 2023

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.

Okay, let's try that. I had a commit in the queue that initializes the variable to NULL, which I think would also get rid of the problem (if not I'd be very surprised) but since I didn't push that let's first try it with your idea.

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Jan 28, 2023

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.

Hm, looks like it doesn't help, the error is still there. So instead let me try this with the (much simpler) "= NULL" solution, which has to work. In fact, I may also drop the flag variable idea, and solve the warning about bool by writing X ? 1 : 0 instead of X != 0.

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
}
JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
POKE(1, res);
if (oparg & 1) { PUSH(res2); }

@gvanrossum gvanrossum Jan 26, 2023

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.

Here (I think).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
iritkatriel
iritkatriel reviewed Jan 26, 2023
View reviewed changes
Comment thread Tools/cases_generator/generate_cases.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum added 3 commits January 26, 2023 08:45
@gvanrossum
Fix two bugs in case generator
6a03438 
- UndefinedLocalError when generating metadata for an 'op'
- Accidental newline inserted in test_generator.py
@gvanrossum
Merge branch 'fix-op-metadata' into cond-cases
2310916
@gvanrossum
Blackify
a0fa513
iritkatriel
iritkatriel reviewed Jan 26, 2023
View reviewed changes
Comment thread Tools/cases_generator/test_generator.py Outdated
Comment on lines +494 to +496
POKE(1, xx);
if (oparg & 2) { PUSH(output); }
PUSH(zz);

@iritkatriel iritkatriel Jan 26, 2023

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.

Could we stick with PEEK and POKE and here have something like this?

Suggested change
POKE(1, xx);
if (oparg & 2) { PUSH(output); }
PUSH(zz);
POKE((oparg & 2) + 2, xx);
if (oparg & 2) { POKE((oparg & 2) + 1, output); }
POKE(1, zz);

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Jan 26, 2023

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.

Interesting. This would favor cases where the conditional is at the bottom of the inputs (e.g. a if (...), b, c). But I think it would become quite horrible if you have several conditions, like we have in MAKE_FUNCTION.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Jan 26, 2023

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.

If there are several conditions then the concatenation of the conditions (and the unconditional 1's) gives you the index.
The generator will be simpler, the generated code will have these expressions for indices, but I think that's probably easier to follow than a mixture of pushes and pokes.

Sorry, something went wrong.

Uh oh!

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

@gvanrossum gvanrossum Jan 26, 2023

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'll play around with that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@gvanrossum gvanrossum Jan 26, 2023

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.

Have a look. The generator sure is a lot simpler, I think I can live with the more complicated generated code. What made this solution so nice is that we already had the symbolic size -- the condition just gets added into that. Thanks for the nudge!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel reacted with hooray emoji
All reactions
  • 🎉 1 reaction
gvanrossum added 2 commits January 26, 2023 09:30
@gvanrossum
Merge remote-tracking branch 'origin/main' into cond-cases
8aeaf01
@gvanrossum
Use Irit's idea
6240934
gvanrossum
gvanrossum commented Jan 26, 2023
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.

Notes to self.

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
At most one of these will be non-zero / non-empty.
"""
if effect.size:
assert not effect.cond, "Manual effects should be conditional"

@gvanrossum gvanrossum Jan 26, 2023

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.

This message is incorrect.

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
Comment on lines +171 to +172
if "boerenkool" in dst.name or "boerenkool" in src.name:
breakpoint()

@gvanrossum gvanrossum Jan 26, 2023

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.

Debug output accidentally left in.

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
Comment on lines +181 to +185
# elif m := re.match(r"^POP\(\)$", dst.name):
# if src.cond:
# self.emit(f"if ({src.cond}) {{ PUSH({cast}{src.name}); }}")
# else:
# self.emit(f"PUSH({cast}{src.name});")

@gvanrossum gvanrossum Jan 26, 2023

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.

Delete commented-out block.

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
else:
# Write output register assignments
for oeffect, reg in zip(self.output_effects, self.output_registers):
src = StackEffect(reg, "")

@gvanrossum gvanrossum Jan 26, 2023

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.

Don't need this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
gvanrossum added 2 commits January 26, 2023 15:53
@gvanrossum
Cleanup
d78e2a7
@gvanrossum
Change order in LOAD_ATTR output effect
e69c1f3
gvanrossum
gvanrossum commented Jan 27, 2023
View reviewed changes
Comment thread Python/generated_cases.c.h Outdated
Comment on lines +1797 to +1799
STACK_GROW(((oparg & 1) != 0));
POKE(1, res);
if (oparg & 1) { POKE(1 + ((oparg & 1) != 0), res2); }

@gvanrossum gvanrossum Jan 27, 2023

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.

It's unfortunate that all new versions test for oparg & 1 several times, whereas the original version tested that condition exactly once.

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel Jan 27, 2023

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.

I assumed the compiler would do this, but we could assign ((oparg& i) != 0) to something. I think it’s only used as a condition in one place, in other places it’s just int arithmetic.

Sorry, something went wrong.

Uh oh!

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

iritkatriel commented Jan 27, 2023

Copy link
Copy Markdown
Member

I don't understand what this warning is about:

comparison of constant ‘0’ with boolean expression is always true [-Wbool-compare]
All reactions

Sorry, something went wrong.

Uh oh!

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

gvanrossum commented Jan 28, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I don't understand what this warning is about:

comparison of constant ‘0’ with boolean expression is always true [-Wbool-compare]

I assume it's about this line (L1797):

STACK_GROW(((oparg & 1) != 0));

The compiler probably reasons that x >= 0 where x has type bool is always true and is trying to warn you that you've got a trivial condition (the expansion of STACK_GROW(n) uses assert(n >= 0) and presumably assert(x) expands to something like if (!x) ...).

That's silly since it doesn't warn us about the equally trivial 1 >= 0 that appears in the expansion of STACK_GROW(1) -- I'm guessing it's more likely to warn based on types rather than values.

I'm not sure what to do about it.

PS. I'm still looking into your suggestion to assign the condition to a flag variable, it's a bit messy.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gvanrossum
Fix type annotations to satisfy pyright (and mypy mostly)
c5529c6
@gvanrossum

gvanrossum commented Jan 28, 2023

Copy link
Copy Markdown
Member Author

Here's the new version. Let me know if you like it enough to go ahead -- if not, I can easily roll it back. Note that I also fixed a bunch of typing issues (which caught one genuine bug).
All reactions

Sorry, something went wrong.

Uh oh!

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

iritkatriel commented Jan 28, 2023

Copy link
Copy Markdown
Member

Here's the new version. Let me know if you like it enough to go ahead -- if not, I can easily roll it back. Note that I also fixed a bunch of typing issues (which caught one genuine bug).

I think this is easier to read. The warning about the bool comparison is gone too. There's just the one about res being potentially unassigned.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gvanrossum
gvanrossum commented Jan 28, 2023
View reviewed changes
Comment thread Python/bytecodes.c
// 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 *container, *start, *stop, *v, *lhs, *rhs, *res2;

@gvanrossum gvanrossum Jan 28, 2023

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.

We will also need static int res2_exists, and a bunch of other things, before I land this.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@gvanrossum
Solve uninitalized variable warning using = NULL
37e3b40
@gvanrossum
Use 'X ? 1 : 0' instead of 'X != 0'
81e5236 
This hopefully avoids a silly compiler warning.
@gvanrossum
gvanrossum force-pushed the cond-cases branch from 8f4219a to 81e5236 Compare January 28, 2023 23:16
@gvanrossum

gvanrossum commented Jan 28, 2023

Copy link
Copy Markdown
Member Author

Finally this has no compiler warnings on GitHub. I prefer this version, the changes to generate_cases.py are simpler than the version with flags.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
iritkatriel
iritkatriel approved these changes Jan 29, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jan 29, 2023
@gvanrossum
gvanrossum merged commit f5a3d91 into python:main Jan 30, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Jan 30, 2023
@gvanrossum
gvanrossum deleted the cond-cases branch January 30, 2023 01:28
mdboom pushed a commit to mdboom/cpython that referenced this pull request Jan 31, 2023
@gvanrossum @mdboom
pythongh-98831: Support conditional effects; use for LOAD_ATTR (pytho…
64eb063 
…n#101333)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iritkatriel iritkatriel iritkatriel approved these changes
@brandtbucher brandtbucher Awaiting requested review from brandtbucher

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 @iritkatriel @bedevere-bot
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.