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

[mypyc] Merge yield_from_except_op #9660

Merged
merged 8 commits into from
Nov 4, 2020
Merged

[mypyc] Merge yield_from_except_op #9660

merged 8 commits into from
Nov 4, 2020

Conversation

TH3CHARLie
Copy link
Collaborator

relates mypyc/mypyc#753

This PR merges yield_from_except_op into the new IR, including several changes:

  • A change to C wrapper's signature since the new IR currently has no unified way to represent the address of a pointer.
  • A change to LoadAddress, allowing it to load local reg's address.
  • A change to uninit pass, suppressing checks on LoadAddress

@TH3CHARLie TH3CHARLie requested a review from JukkaL October 29, 2020 15:20
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks -- this is a tricky one! I have some thoughts about potentially making this cleaner. It's kind of awkward to have to use casts from PyObject * to PyObject **.

mypyc/lib-rt/CPy.h Outdated Show resolved Hide resolved
mypyc/lib-rt/misc_ops.c Outdated Show resolved Hide resolved
mypyc/primitives/misc_ops.py Outdated Show resolved Hide resolved
@@ -45,7 +46,8 @@ def split_blocks_at_uninits(env: Environment,
# initialized is an operand to something other than a
# check that it is defined, insert a check.
if (isinstance(src, Register) and src not in defined
and not (isinstance(op, Branch) and op.op == Branch.IS_ERROR)):
and not (isinstance(op, Branch) and op.op == Branch.IS_ERROR)
and not isinstance(op, LoadAddress)):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this? Add comment explaining this, since this is a bit surprising. I wonder if we can somehow get rid of this special case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We need this because we are taking the address of an uninitialized object, the automatically inserted initialization check will produce incorrect code, leading the control flow directly to uninitialized error rather than the following load address and other ops.

mypyc/ir/ops.py Show resolved Hide resolved
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 2, 2020

Can you check that there is some existing case that uses the primitive? If not, it makes sense to add one (but I assume that this is tested somewhere).

@TH3CHARLie
Copy link
Collaborator Author

Can you check that there is some existing case that uses the primitive? If not, it makes sense to add one (but I assume that this is tested somewhere).

yes, there's testYield for this(but I think it's not completely for this op itself but also so other yield and exception stuff)

@TH3CHARLie
Copy link
Collaborator Author

I've made some updates according to your review, including using object_pointer_rprimitive and a new IR test. Unfortunately, the special case in the uninit pass is somewhat inevitable

@TH3CHARLie TH3CHARLie requested a review from JukkaL November 3, 2020 11:45
Copy link
Collaborator

@JukkaL JukkaL 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 updates! Looks good.

I'd suggest dropping the test case (but even then it has been useful to see what sort of code is being generated).

mypyc/test-data/irbuild-basic.test Outdated Show resolved Hide resolved
@TH3CHARLie TH3CHARLie merged commit 34dc670 into python:master Nov 4, 2020
@TH3CHARLie TH3CHARLie deleted the yield_from_except-op branch November 4, 2020 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants