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

C++: Implement proper coroutine support in IR #16301

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

MathiasVP
Copy link
Contributor

This PR fixes the IR generation for co_await and co_yield. We added IR support for these in #16187. However, we didn't quite capture the right semantics of the constructs. This PR fixes this by generating the (hopefully) correct semantics by making use of the children of co_await and co_yield as emitted by our frontend.

This gets rid of all the dead ends in the IR. I haven't quite figured out why we get new nonUniqueIRVariable inconsistencies, though (and whether this is a QL or extractor bug).

@github-actions github-actions bot added the C++ label Apr 22, 2024
Co-authored-by: Jeroen Ketema <[email protected]>
@MathiasVP MathiasVP marked this pull request as ready for review April 23, 2024 07:13
@MathiasVP MathiasVP requested a review from a team as a code owner April 23, 2024 07:13
Comment on lines +190 to +191
tag = ResultCopyTag() and result = "ResultCopy"
or
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a drive-by fix I did while debugging. This tag didn't have a result previously.

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Apr 23, 2024
@jketema
Copy link
Contributor

jketema commented Apr 23, 2024

This gets rid of all the dead ends in the IR. I haven't quite figured out why we get new nonUniqueIRVariable inconsistencies, though (and whether this is a QL or extractor bug).

I think this is related to the issue I reported to our frontend provider (but let me double check that).

@MathiasVP
Copy link
Contributor Author

I think this is related to the issue I reported to our frontend provider (but let me double check that).

Awesome. That will save me half a morning of debugging then 🎉

@jketema
Copy link
Contributor

jketema commented Apr 23, 2024

I think this is related to the issue I reported to our frontend provider (but let me double check that).

Awesome. That will save me half a morning of debugging then 🎉

Yeah, those are exactly the local variables that we filed an issue for with our frontend provider.

If these are not causing any serious problems, I propose with just add a comment to the open issue on this that once we receive a patch we should also fix this (if that's not automatic).

@jketema
Copy link
Contributor

jketema commented Apr 23, 2024

One question: how do the translations of the reuse expression look (they are a bit hard to find)? In the intermediate state we had before this PR, the use and reuse we in separate blocks. Is that solved now?

@jketema
Copy link
Contributor

jketema commented Apr 23, 2024

One question: how do the translations of the reuse expression look (they are a bit hard to find)? In the intermediate state we had before this PR, the use and reuse we in separate blocks. Is that solved now?

Ah, they still seem to be in different blocks, but the order of the blocks seems such that we get something that is initialised.

@MathiasVP
Copy link
Contributor Author

Ah, they still seem to be in different blocks, but the order of the blocks seems such that we get something that is initialised.

Indeed. It seems like the frontend is doing sensible things here 🎉

@MathiasVP
Copy link
Contributor Author

If these are not causing any serious problems, I propose with just add a comment to the open issue on this that once we receive a patch we should also fix this (if that's not automatic).

I would expect this to be fixed automagically, but let's note it in the internal issue anyway 👍

@MathiasVP MathiasVP merged commit 553c09a into github:main Apr 23, 2024
15 of 16 checks passed
@jketema
Copy link
Contributor

jketema commented Apr 23, 2024

If these are not causing any serious problems, I propose with just add a comment to the open issue on this that once we receive a patch we should also fix this (if that's not automatic).

I would expect this to be fixed automagically, but let's note it in the internal issue anyway 👍

Added.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C++ no-change-note-required This PR does not need a change note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants