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.

Sign up for GitHub

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

Jump to bottom

[AutoDiff] IRGen differentiability witness tables #28067

Merged
merged 4 commits into from Nov 7, 2019
Merged

[AutoDiff] IRGen differentiability witness tables #28067

merged 4 commits into from Nov 7, 2019

Conversation

marcrasi
Copy link
Collaborator

@marcrasi marcrasi commented Nov 5, 2019 •
edited

Summary of changes:

  • Add LinkEntity for SILDifferentiabilityWitnessTable.
  • Add IRGenModule::emitSILDifferentiabilityWitness & IRGenModule::getAddrOfDifferentiabilityWitness, which do the actual work of emitting the IR.
  • Add TBDGen for the differentiability witnesses. Also some factoring duplicated code out into a common function.

I think this resolves TF-930, by making TBDGen incorporate the VJP/JVP generic signatures in its calculation.

dan-zheng
dan-zheng approved these changes Nov 5, 2019
View reviewed changes
Copy link
Collaborator

@dan-zheng dan-zheng left a comment •
edited

Choose a reason for hiding this comment

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

Nice TBDGen refactoring!
Closing TF-930 sounds good if it's no longer reproducible.

lib/TBDGen/TBDGenVisitor.h Outdated Show resolved Hide resolved
lib/IRGen/Linking.cpp Outdated Show resolved Hide resolved
lib/IRGen/Linking.cpp Outdated Show resolved Hide resolved
lib/IRGen/Linking.cpp Outdated Show resolved Hide resolved
lib/IRGen/Linking.cpp Outdated Show resolved Hide resolved
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please clean test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

The build error looks related to the monorepo transition. I expect that rebasing this on top of the transition will fix it. I'll wait until the transition gets merged into tensorflow (it should be really soon, right?), rebase this PR, and then rerun CI on it.

@marcrasi marcrasi changed the base branch from tensorflow to tensorflow-merge November 5, 2019 19:39
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please test tensorflow

1 similar comment
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please clean test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please test tensorflow

2 similar comments
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 5, 2019

@swift-ci please test tensorflow

rxwei
rxwei approved these changes Nov 5, 2019
View reviewed changes
Copy link
Member

@rxwei rxwei 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 for keeping me in the loop!

dan-zheng
dan-zheng reviewed Nov 6, 2019
View reviewed changes
lib/AST/PrettyStackTrace.cpp Outdated

void swift::printDifferentiabilityWitnessDescription(
llvm::raw_ostream &out, const SILDifferentiabilityWitnessKey key,
ASTContext &Context, bool addNewline) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ASTContext &Context, bool addNewline) {
bool addNewline) {

ASTContext &Context is unused, please remove the argument here and the corresponding field from PrettyStackTraceDifferentiabilityWitness.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@marcrasi marcrasi changed the base branch from tensorflow-merge to tensorflow November 7, 2019 17:58
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow

1 similar comment
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow

2 similar comments
@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow linux

@marcrasi
Copy link
Collaborator Author

marcrasi commented Nov 7, 2019

@swift-ci please test tensorflow

@marcrasi marcrasi merged commit d636456 into apple:tensorflow Nov 7, 2019
@marcrasi marcrasi deleted the diff-wit-irgen branch November 7, 2019 21:27
@dan-zheng dan-zheng mentioned this pull request Nov 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dan-zheng dan-zheng dan-zheng approved these changes

@rxwei rxwei rxwei approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marcrasi @rxwei @dan-zheng