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

Implement more precise memory analysis for LoopIR.Free #223

Merged
merged 2 commits into from
Aug 4, 2022
Merged

Conversation

yamaguchi1024
Copy link
Member

This PR updates the insertion of LoopIR.Free from

for ...:
  x : R[...]
  ....
  free(x)

to

for ...:
  x : R[...]
  x = ...
  z = x * ... # last use of x
  free(x)
  ....

@yamaguchi1024 yamaguchi1024 enabled auto-merge (squash) August 2, 2022 23:27
@gilbo
Copy link
Contributor

gilbo commented Aug 4, 2022

I'm still not clear on why we would want to make this change. @alexreinking ?

@yamaguchi1024 If the concern here is that you want to be able to reclaim previously used memory, then we should probably do this a different way. One possibility is to do some kind of aggregate static allocation because we can determine that ahead of time. Another possibility (for whatever specific code is prompting this change) is to manually re-use the memory in question. (we even have one scheduling primitive designed to help do that)

Since the user can't currently control where Free is inserted, the chosen policy ought to somehow be universal. I'm not clear on why this change to Free insertion would be universally better, nor why there isn't a universally optimal solution (which would suggest that we need to expose this to users instead...)

@yamaguchi1024
Copy link
Member Author

@gilbo Because users cannot control where Free is inserted, I think Free insertion logic should be as tight as possible. In general, I don't think we want to hold a memory allocation that is not used until the end of the scope.

Copy link
Contributor

@alexreinking alexreinking left a comment

Choose a reason for hiding this comment

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

I think this is the right thing to do in the absence of user scheduling for frees. I didn't check the algorithm here super carefully, but I'm a little surprised none of the golden outputs changed.

@yamaguchi1024 yamaguchi1024 merged commit c758bb7 into master Aug 4, 2022
@yamaguchi1024 yamaguchi1024 deleted the free branch August 4, 2022 18:18
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.

3 participants