Skip to content

GH-118095: Handle RETURN_GENERATOR in tier 2 #118180

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

Merged
merged 10 commits into from
Apr 25, 2024

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Apr 23, 2024

@markshannon markshannon marked this pull request as ready for review April 24, 2024 11:43
Copy link
Member

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

Why not add a test? E.g.

    def test_return_generator(self):
        def gen():
            yield None
        def testfunc(n):
            for i in range(n):
                gen()
        res, ex = self._run_with_optimizer(testfunc, 20)
        self.assertIsNotNone(ex)
        self.assertIn("_RETURN_GENERATOR", get_opnames(ex))

@@ -369,7 +369,7 @@ eliminate_pop_guard(_PyUOpInstruction *this_instr, bool exit)
static PyCodeObject *
get_code(_PyUOpInstruction *op)
{
assert(op->opcode == _PUSH_FRAME || op->opcode == _POP_FRAME);
assert(op->opcode == _PUSH_FRAME || op->opcode == _POP_FRAME || op->opcode == _RETURN_GENERATOR);
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit inelegant that all places that treat _PUSH_FRAME and _POP_FRAME special now also have to check for _RETURN_GENERATOR. Not sure what to do about it. :-(

Copy link
Member Author

Choose a reason for hiding this comment

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

Once #118095 is done we can add a flag for all uops that push or pop frames.

ctx->frame->stack_pointer = stack_pointer;
frame_pop(ctx);
stack_pointer = ctx->frame->stack_pointer;
OUT_OF_SPACE_IF_NULL(res = sym_new_unknown(ctx));
Copy link
Member

Choose a reason for hiding this comment

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

Am I correct that this is the only line that differs from _POP_FRAME? The duplication of so much code is unfortunate. At the same time I don't see a better solution. Maybe add a comment to both explaining they need to be kept in sync?

Copy link
Member Author

Choose a reason for hiding this comment

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

A good candidate for refactoring, but I'd like to get #118095 done first so that we more easily see the common patterns.

def testfunc(n):
for i in range(n):
gen()
res, ex = self._run_with_optimizer(testfunc, 20)
Copy link
Member

Choose a reason for hiding this comment

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

I'd assert the value of res as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@markshannon
Copy link
Member Author

The four failing tests are the usual suspects.

@markshannon markshannon merged commit f180b31 into python:main Apr 25, 2024
48 of 52 checks passed
@markshannon markshannon deleted the return-gen-tier-2 branch April 25, 2024 10:34
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