Skip to content

[AutoDiff] Parameter indices data structure overhaul #24761

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 11 commits into from
May 14, 2019

Conversation

rxwei
Copy link
Contributor

@rxwei rxwei commented May 14, 2019

Introduce AutoDiffIndexSubset

This PR overhauls data structures used for parameter indices in the AutoDiff infrastructure in SIL.

Previously, we used llvm::SmallBitVector to represent differentiation parameter indices in both AST and SIL. It was not efficient, and most importantly there's no way to put this in an instruction without causing memory leaks.

This change replaces all uses of llvm::SmallBitVector in SIL AutoDiff code paths with a ASTContext-uniqued AutoDiffIndexSubset * where bits are stored as trailing objects.

AutoDiffIndexSubset does not have "parameter indices" in its name because it is not only designed for parameter indices, but also for result indices as we move to supporting multi-result differentiation. AutoDiffIndexSubset has set operations like isSubsetOf, isSupersetOf, and contains, but it also has a special capacity property. All differentiable function's parameter indices data should store the number of parameters as capacity, so that the differentiation transform won't need special logic to check whether an index is out of range.

Another minor change is the module format layout of SILDifferentiableAttr. It used to store parameter indices as consecutive bool bits, but now stores numeric parameter indices directly for efficiency.

It will be necessary to refactor or eliminate AutoDiffParameterIndices to make use of AutoDiffIndexSubset. AutoDiffParameterIndices is at the AST level, so it is not in the scope for this PR.

Unblocks #23482. Partially resolves TF-67.

@rxwei rxwei added the tensorflow This is for "tensorflow" branch PRs. label May 14, 2019
@rxwei rxwei force-pushed the parameter-indices branch from 241fee3 to 81582bd Compare May 14, 2019 11:13
@rxwei rxwei force-pushed the parameter-indices branch from 81582bd to 4f08e57 Compare May 14, 2019 11:14
@rxwei rxwei requested review from marcrasi and dan-zheng May 14, 2019 11:21
@rxwei rxwei marked this pull request as ready for review May 14, 2019 11:36
@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow

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.

Hurray!

@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

3 similar comments
@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@rxwei rxwei requested a review from bartchr808 May 14, 2019 12:16
@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

4 similar comments
@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@rxwei
Copy link
Contributor Author

rxwei commented May 14, 2019

@swift-ci please test tensorflow Linux

@dan-zheng
Copy link
Contributor

@swift-ci Please test tensorflow linux

@rxwei rxwei merged commit 3d3f9e4 into swiftlang:tensorflow May 14, 2019
@rxwei rxwei deleted the parameter-indices branch May 14, 2019 16:55
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