Skip to content

[AutoDiff] Partially fix AD control flow memory leaks #25294

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

Merged
merged 5 commits into from
Jun 7, 2019

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented Jun 6, 2019

This PR partially fixes the incorrect omission of memory cleanup in AdjointEmitter.

Cleanups should be created per basic block in an adjoint function, starting with its arguments. Like function parameter arguments in the adjoint entry block, every phi argument except the pullback struct in a non-entry adjoint block also needs to possess a cleanup. We let the existing adjoint emission logic accumulate the cleanups until we are about to branch, where we disable the top-level cleanup for arguments in the terminator instruction and apply all of their child cleanups recursively. This logic is more general than the original single-block differentiation logic, and we should refactor AdjointEmitter::run() to eliminate special logic for adjoint entry and exit blocks as much as possible.

bb1(x, y, pbs):
    v0 = pbs.0(x, y)
    v1 = pbs.1(v0)
    release_value x
    release_value y
    release_value v0
    switch pbs.predessor, case #case1: bb2, case #case2: bb3
bb2_tramp(...):
    br bb2(v1)
bb2(...):
    ...
bb3_tramp(...):
    br bb3(v1)
bb3(...):
    ...

This PR also unifies the argument order of the adjoint entry block with that of other adjoint blocks, making the pullback struct always the last argument. This helps us reduce special logic for the adjoint entry block later.

@rxwei rxwei added the tensorflow This is for "tensorflow" branch PRs. label Jun 6, 2019
@rxwei rxwei requested review from dan-zheng and bartchr808 June 6, 2019 23:04
Copy link
Contributor

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

Nice! Do you know what's causing the remaining leaks (adjoint values/buffers local to an adjoint block)?

@rxwei
Copy link
Contributor Author

rxwei commented Jun 6, 2019

@swift-ci please test tensorflow

@rxwei
Copy link
Contributor Author

rxwei commented Jun 6, 2019

Do you know what's causing the remaining leaks (adjoint values/buffers local to an adjoint block)?

Not sure yet. Debugging.

@rxwei
Copy link
Contributor Author

rxwei commented Jun 7, 2019

@swift-ci please test tensorflow

1 similar comment
@rxwei
Copy link
Contributor Author

rxwei commented Jun 7, 2019

@swift-ci please test tensorflow

@rxwei rxwei merged commit c705359 into swiftlang:tensorflow Jun 7, 2019
@rxwei rxwei deleted the memory-leak-fix branch June 7, 2019 00:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tensorflow This is for "tensorflow" branch PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants