gh-98831: Support conditional effects; use for LOAD_ATTR #101333

gvanrossum · 2023-01-26T00:47:28Z

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

(Semantics aren't there yet, and the test fails.)

First use of conditional stack effects.

gvanrossum · 2023-01-26T05:19:28Z

Python/bytecodes.c

@@ -1438,13 +1438,11 @@ dummy_func(
            PREDICT(JUMP_BACKWARD);
        }

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


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

gvanrossum · 2023-01-26T05:21:55Z

Python/generated_cases.c.h

@@ -1745,11 +1745,13 @@

        TARGET(LOAD_ATTR) {
            PREDICTED(LOAD_ATTR);
+            PyObject *owner = PEEK(1);
+            PyObject *res;
+            PyObject *res2;


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.

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?

I'll just initialize it to NULL. Perhaps some later phase of optimization will recognize that for the dead value it is.

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.

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.

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.

gvanrossum · 2023-01-26T05:22:09Z

Python/generated_cases.c.h

            }
-            JUMPBY(INLINE_CACHE_ENTRIES_LOAD_ATTR);
+            POKE(1, res);
+            if (oparg & 1) { PUSH(res2); }


Here (I think).

Tools/cases_generator/generate_cases.py

- UndefinedLocalError when generating metadata for an 'op' - Accidental newline inserted in test_generator.py

iritkatriel · 2023-01-26T17:04:02Z

Tools/cases_generator/test_generator.py

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


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

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.

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.

I'll play around with that.

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!

gvanrossum

Notes to self.

gvanrossum · 2023-01-26T20:05:55Z

Tools/cases_generator/generate_cases.py

@@ -59,7 +61,10 @@ def effect_size(effect: StackEffect) -> tuple[int, str]:
    At most one of these will be non-zero / non-empty.
    """
    if effect.size:
+        assert not effect.cond, "Manual effects should be conditional"


This message is incorrect.

gvanrossum · 2023-01-26T20:06:26Z

Tools/cases_generator/generate_cases.py

+        if "boerenkool" in dst.name or "boerenkool" in src.name:
+            breakpoint()


Debug output accidentally left in.

gvanrossum · 2023-01-26T20:06:46Z

Tools/cases_generator/generate_cases.py

+        # 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});")


Delete commented-out block.

gvanrossum · 2023-01-26T20:07:29Z

Tools/cases_generator/generate_cases.py

@@ -320,6 +354,7 @@ def write(self, out: Formatter) -> None:
        else:
            # Write output register assignments
            for oeffect, reg in zip(self.output_effects, self.output_registers):
+                src = StackEffect(reg, "")


Don't need this.

gvanrossum · 2023-01-27T00:47:27Z

Python/generated_cases.c.h

+            STACK_GROW(((oparg & 1) != 0));
+            POKE(1, res);
+            if (oparg & 1) { POKE(1 + ((oparg & 1) != 0), res2); }


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

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.

iritkatriel · 2023-01-27T11:07:29Z

I don't understand what this warning is about:

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

gvanrossum · 2023-01-28T02:12:46Z

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.

gvanrossum · 2023-01-28T02:44:57Z

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

iritkatriel · 2023-01-28T11:39:38Z

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.

gvanrossum · 2023-01-28T22:49:10Z

Python/bytecodes.c

@@ -51,7 +51,7 @@

 // 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;


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

This hopefully avoids a silly compiler warning.

gvanrossum · 2023-01-28T23:35:11Z

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.

…n#101333)

gvanrossum added 5 commits January 25, 2023 16:40

Add parser support for conditional stack effects

af97a7e

(Semantics aren't there yet, and the test fails.)

Fix a typo in test_generator.py that made it into main :-(

fbcdde3

Make conditional effects work, in theory

27246e9

Convert LOAD_ATTR to new format

667630e

First use of conditional stack effects.

Conditionalize effect_size()

69dbc5e

bedevere-bot added the awaiting core review label Jan 26, 2023

bedevere-bot mentioned this pull request Jan 26, 2023

Generate the interpreter #98831

Closed

gvanrossum marked this pull request as ready for review January 26, 2023 00:47

gvanrossum requested review from brandtbucher and iritkatriel January 26, 2023 00:48

gvanrossum added the skip news label Jan 26, 2023

gvanrossum commented Jan 26, 2023

View reviewed changes

iritkatriel reviewed Jan 26, 2023

View reviewed changes

Tools/cases_generator/generate_cases.py Outdated Show resolved Hide resolved

gvanrossum added 3 commits January 26, 2023 08:45

Fix two bugs in case generator

6a03438

- UndefinedLocalError when generating metadata for an 'op' - Accidental newline inserted in test_generator.py

Merge branch 'fix-op-metadata' into cond-cases

2310916

Blackify

a0fa513

iritkatriel reviewed Jan 26, 2023

View reviewed changes

gvanrossum added 2 commits January 26, 2023 09:30

Merge remote-tracking branch 'origin/main' into cond-cases

8aeaf01

Use Irit's idea

6240934

gvanrossum commented Jan 26, 2023

View reviewed changes

gvanrossum added 2 commits January 26, 2023 15:53

Cleanup

d78e2a7

Change order in LOAD_ATTR output effect

e69c1f3

gvanrossum commented Jan 27, 2023

View reviewed changes

Fix type annotations to satisfy pyright (and mypy mostly)

c5529c6

gvanrossum commented Jan 28, 2023

View reviewed changes

Solve uninitalized variable warning using = NULL

37e3b40

Use 'X ? 1 : 0' instead of 'X != 0'

81e5236

This hopefully avoids a silly compiler warning.

gvanrossum force-pushed the cond-cases branch from 8f4219a to 81e5236 Compare January 28, 2023 23:16

iritkatriel approved these changes Jan 29, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jan 29, 2023

gvanrossum merged commit f5a3d91 into python:main Jan 30, 2023

bedevere-bot removed the awaiting merge label Jan 30, 2023

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

pythongh-98831: Support conditional effects; use for LOAD_ATTR (pytho…

64eb063

…n#101333)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-98831: Support conditional effects; use for LOAD_ATTR #101333

gh-98831: Support conditional effects; use for LOAD_ATTR #101333

gvanrossum commented Jan 26, 2023 •

edited

Loading

gvanrossum Jan 26, 2023

gvanrossum Jan 26, 2023

iritkatriel Jan 28, 2023

gvanrossum Jan 28, 2023

iritkatriel Jan 28, 2023

gvanrossum Jan 28, 2023

gvanrossum Jan 28, 2023

gvanrossum Jan 26, 2023

iritkatriel Jan 26, 2023

gvanrossum Jan 26, 2023

iritkatriel Jan 26, 2023

gvanrossum Jan 26, 2023

gvanrossum Jan 26, 2023

gvanrossum left a comment

gvanrossum Jan 26, 2023

gvanrossum Jan 26, 2023

gvanrossum Jan 26, 2023

gvanrossum Jan 26, 2023

gvanrossum Jan 27, 2023

iritkatriel Jan 27, 2023

iritkatriel commented Jan 27, 2023

gvanrossum commented Jan 28, 2023 •

edited

Loading

gvanrossum commented Jan 28, 2023

iritkatriel commented Jan 28, 2023

gvanrossum Jan 28, 2023

gvanrossum commented Jan 28, 2023

		if "boerenkool" in dst.name or "boerenkool" in src.name:
		breakpoint()

gh-98831: Support conditional effects; use for LOAD_ATTR #101333

gh-98831: Support conditional effects; use for LOAD_ATTR #101333

Conversation

gvanrossum commented Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iritkatriel commented Jan 27, 2023

gvanrossum commented Jan 28, 2023 • edited Loading

gvanrossum commented Jan 28, 2023

iritkatriel commented Jan 28, 2023

Choose a reason for hiding this comment

gvanrossum commented Jan 28, 2023

gvanrossum commented Jan 26, 2023 •

edited

Loading

gvanrossum commented Jan 28, 2023 •

edited

Loading