Skip to content

[AutoDiff] @_alwaysEmitIntoClient registration tests #29129

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

Conversation

marcrasi
Copy link

Here are some negative tests demonstrating the problem in TF-1103.

What's happening is:

  • When the differentiation pass is processing gradient(at: 0, in: f), it finds the differentiability config arising from the @derivative(of: f), and creates a corresponding differentiability witness declaration.
  • The linker tries to link references to that witness to an actual definition.
  • There is no actual definition because the witness linkage is PublicNonABI, which means that the witness does not get emitted into the library.

Some possible fixes are:

  • Make it so that the differentiability witness is Public instead of PublicNonABI. An obstacle for this approach is that TBDGen currently bases its decision on whether to emit a TBD entry on the linkage of the original function. So TBD doesn't won't emit a TBD entry for the witness for a PublicNonABI function. We could overcome this obstacle by teaching TBDGen to use the linkage of the derivative function instead of the linkage of the original function.
  • Teach the differentiation pass about PublicNonABI witnesses. Specifically:
    • Put information about the witness linkage in the DerivativeFunctionConfigurationList.
    • Make the pass create a decl with the appropriate linkage based on that information.
    • Teach something to deserialize these witnesses. (Maybe SIL/Linker.cpp is the correct place to do this, if the linking pass runs after differentiation. However, if the linking pass runs before differentiation, maybe we'll need the differentiation pass to deserialize these).

My initial thought is that the second approach is better because we want the visibility/linkage/etc of the derivative functions to always be as close to the original functions as possible.

@marcrasi marcrasi requested a review from dan-zheng January 10, 2020 21:45
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 tests!

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

4 similar comments
@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@@ -0,0 +1,21 @@
// TODO(TF-1103): Fix this test so that there is not a linker error. Then, move this test to
// cross_module_derivative_attr_e2e.swift.
Copy link
Member

Choose a reason for hiding this comment

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

I believe that this would belong in the validation tests directory

Copy link
Author

Choose a reason for hiding this comment

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

What tests should go there? We haven't put any autodiff tests there -- we've put them all under tests/, even ones that build modules like this one.

@marcrasi
Copy link
Author

@swift-ci please test tensorflow macos

1 similar comment
@ematejska
Copy link
Contributor

@swift-ci please test tensorflow macos

@ematejska
Copy link
Contributor

@swift-ci please test tensorflow clean linux

@ematejska
Copy link
Contributor

@swift-ci please clean test tensorflow linux

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@marcrasi
Copy link
Author

@swift-ci please test tensorflow

1 similar comment
@marcrasi
Copy link
Author

@swift-ci please test tensorflow

@marcrasi marcrasi merged commit 709d7d5 into swiftlang:tensorflow Feb 10, 2020
@marcrasi marcrasi deleted the always-emit-into-client-negative-test branch February 10, 2020 22:48
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.

4 participants