Skip to content

Clean up ret_mode and region in Lambda.lfunction #2985

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

mshinwell
Copy link
Collaborator

...and consequentially contains_no_escaping_local_allocs in Flambda 2, which was unused.

Simplif seemed to be the only place relying on this field. @riaqn suggested that ret_mode could be used instead, which seems like it should be correct, but this part of the diff needs checking carefully. The rest is straightforward.

@mshinwell mshinwell added flambda2 Prerequisite for, or part of, flambda2 lambda Lambda language changes labels Aug 28, 2024
@mshinwell mshinwell requested a review from riaqn August 28, 2024 09:02
@riaqn riaqn mentioned this pull request Aug 28, 2024
Copy link
Contributor

@riaqn riaqn left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

Please hold off merging - I'd like to push commit to clean up the comments.

@mshinwell
Copy link
Collaborator Author

I'd like @lpw25 to read the changes to Simplif (not commit-by-commit, read the whole at once) and the changes to the comments. Otherwise this is ok. Thanks.

@riaqn
Copy link
Contributor

riaqn commented Aug 28, 2024

@lpw25 and I talked, and we think we want this PR to fix things more thoroughly:

  • We will remove ret_mode and region from lfunction.
  • We add a field allocate_in_caller_region (or some name similar). It's more precise than ret_mode, because a function with ret_mode=local might not allocate in caller's region (think fun (local_ x) -> x), whereas this new field should give a definite answer. It will be set to true by exclave.
  • Propagate similar name changes to flambda2.

I will create JIRA ticket to track this, and hope to do it in the next few weeks.

@riaqn riaqn marked this pull request as draft August 28, 2024 15:33
@riaqn riaqn changed the title Remove "region" from Lambda.lfunction Clean up ret_mode and region in Lambda.lfunction Aug 28, 2024
@mshinwell
Copy link
Collaborator Author

I will have to look at the discussion again but I think there were some problems with inadequate precision of locality information that caused trouble in #1271. Maybe this new idea will solve that. We should revisit that discussion when work recommences on this PR.

@riaqn riaqn force-pushed the flambda2-stop-using-region-field branch from 4fd39ab to 0f174bf Compare February 12, 2025 17:21
@riaqn
Copy link
Contributor

riaqn commented Feb 12, 2025

I had another discussion with @lpw25 . Recollection:

  • The type checker should track whether a function allocates in caller's region (ACR), and reflect that information in functions' types. This is to reject code that seem like not ACR but actually do and surprise users. In particular, tail-calling an ACR make the current function ACR as well.
  • This is currently conservatively approxmated by the return mode of arrow types (and copied to ret_mode in lfunction). In the future once we introduce n-ary arrows, we will decouple ACR from the mode of the return value, and ACR will only be set by exclave.
  • With the current conservative approximation, the middle-end can be more precise by inspecting function body.

@riaqn riaqn marked this pull request as ready for review February 17, 2025 15:14
Copy link
Contributor

@riaqn riaqn left a comment

Choose a reason for hiding this comment

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

I looked at the changes again, and they seem fine to me.

@mshinwell could you look at my last commit "fix probe handler empty parameters", then we can merge.

@riaqn riaqn force-pushed the flambda2-stop-using-region-field branch from 7a6eeb1 to 51a051a Compare February 18, 2025 09:07
@riaqn riaqn enabled auto-merge (squash) February 18, 2025 09:09
@riaqn riaqn merged commit 1a73a72 into ocaml-flambda:main Feb 18, 2025
23 checks passed
Dreian pushed a commit to Dreian/flambda-backend that referenced this pull request Feb 19, 2025
…#2985)

Co-authored-by: Zesen Qian <github@riaqn.org>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
flambda2 Prerequisite for, or part of, flambda2 lambda Lambda language changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants