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

Model function calls as initializing expressions #3089

Merged
merged 29 commits into from
Aug 24, 2023

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Aug 11, 2023

Start treating function calls as initializing expressions instead of as value expressions.

This required adding support for expression categories. Value bindings and temporary materialization conversions are created where necessary to transition between expression categories. For a function call with a return slot, we speculatively create a materialized temporary before the call and either commit to it or replace it with something else later, once we see how the function call expression is actually used.

This change follows the direction suggested in #3133 for initializing expressions: depending on the return type of a function, the return value will either be initialized in-place or returned directly. This is visible in the semantics IR, which is a little unfortunate but is probably necessary as this is part of the semantics of the program.

Value bindings, temporary materializations, and initialization steps are
created where necessary to transition between expression categories.
chandlerc
chandlerc previously approved these changes Aug 14, 2023
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

I'm not certain about the exact structure we have of semantic nodes with this, but I think it's fine to start here.

My guess is that we may want to look at what is needed to lower this really nicely into LLVM IR and tweak the exact node structure between things like assign, the var or temporary allocation, and the initialize_from nodes, but it's hard to say what would be "right" just yet, and seems better to get the infrastructure in place and then look at tweaks.

Overall LG, some minor questions inline that aren't really blocking either way.

toolchain/lowering/testdata/function/call/var_param.carbon Outdated Show resolved Hide resolved
toolchain/semantics/semantics_node_kind.def Outdated Show resolved Hide resolved
@zygoloid zygoloid changed the title Rough semantic support for expression category conversions. Semantic support for expression category conversions, and modeling calls as initializing expressions Aug 22, 2023
@zygoloid zygoloid changed the title Semantic support for expression category conversions, and modeling calls as initializing expressions Model function calls as initializing expressions Aug 22, 2023
Copy link
Contributor Author

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

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

@chandlerc Sorry for the large influx of new changes since your last review; I wanted to explore this a bit more and see how the direction pans out before committing to it. In the end the changes here aren't exactly an incremental extension of the previous state of the PR, so I think it probably makes more sense to treat this as a from-scratch review rather than an incremental diff compared to the prior PR.

@zygoloid zygoloid dismissed chandlerc’s stale review August 23, 2023 21:10

Applies to substantially different PR.

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

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

I'm pretty tired and so worried I'm missing something, but I think this largely makes sense.

I have some boring style nits in suggested edits, and a question (but not really a concern).

I'm not super delighted by the extensive temporaries, but I understand why this is a reasonably, faithful lowering. And it makes sense to get this and the other semantic things sorted out first, and then revisit things to try to tighten up the emitted LLVM IR.

So I think this LG.

toolchain/semantics/semantics_context.cpp Outdated Show resolved Hide resolved
toolchain/semantics/semantics_context.cpp Outdated Show resolved Hide resolved
zygoloid and others added 2 commits August 24, 2023 12:16
Co-authored-by: Chandler Carruth <[email protected]>
Co-authored-by: Chandler Carruth <[email protected]>
@zygoloid zygoloid added this pull request to the merge queue Aug 24, 2023
Merged via the queue into carbon-language:trunk with commit 1013d17 Aug 24, 2023
6 checks passed
@zygoloid zygoloid deleted the toolchain-expression-category branch August 24, 2023 19:54
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.

None yet

2 participants