Skip to content

[AutoDiff] Change cross-file derivative registration to a frontend flag. #29339

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

Merged
merged 2 commits into from
Jan 23, 2020
Merged

[AutoDiff] Change cross-file derivative registration to a frontend flag. #29339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f57043b
[AutoDiff] Change cross-file derivative registration to a frontend flag.
dan-zheng Jan 21, 2020
1fc4725
Mark relevant differentiable programming flags as `ModuleInterfaceOpt…
dan-zheng Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmake/modules/AddSwift.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1914,7 +1914,7 @@ function(add_swift_target_library name)
# SWIFT_ENABLE_TENSORFLOW
# NOTE(TF-1021): Enable cross-file derivative registration for stdlib.
list(APPEND swiftlib_swift_compile_flags_all
-Xllvm -enable-experimental-cross-file-derivative-registration)
-Xfrontend -enable-experimental-cross-file-derivative-registration)
# SWIFT_ENABLE_TENSORFLOW END

# Collect architecture agnostic SDK linker flags
Expand Down
3 changes: 3 additions & 0 deletions include/swift/Basic/LangOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,9 @@ namespace swift {
// SWIFT_ENABLE_TENSORFLOW END

// SWIFT_ENABLE_TENSORFLOW
/// Whether to enable cross-file derivative registration.
bool EnableExperimentalCrossFileDerivativeRegistration = false;

/// Whether to enable forward mode differentiation.
bool EnableExperimentalForwardModeDifferentiation = false;
// SWIFT_ENABLE_TENSORFLOW END
Expand Down
7 changes: 6 additions & 1 deletion include/swift/Option/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,9 +470,14 @@ def disable_bridging_pch : Flag<["-"], "disable-bridging-pch">,

// Experimental feature options
def enable_experimental_differentiable_programming : Flag<["-"], "enable-experimental-differentiable-programming">,
Flags<[FrontendOption]>,
Flags<[FrontendOption, ModuleInterfaceOption]>,
HelpText<"Enable experimental differentiable programming features">;

// SWIFT_ENABLE_TENSORFLOW
def enable_experimental_cross_file_derivative_registration : Flag<["-"], "enable-experimental-cross-file-derivative-registration">,
Flags<[FrontendOption, ModuleInterfaceOption]>,
HelpText<"Enable experimental cross-file derivative registration">;

// NOTE: This flag will be removed when JVP/differential generation is robust.
def enable_experimental_forward_mode_differentiation : Flag<["-"], "enable-experimental-forward-mode-differentiation">,
Flags<[FrontendOption]>,
Expand Down
15 changes: 5 additions & 10 deletions lib/Frontend/CompilerInvocation.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,15 +240,6 @@ static void SaveModuleInterfaceArgs(ModuleInterfaceOptions &Opts,
return;
ArgStringList RenderedArgs;
for (auto A : Args) {
// SWIFT_ENABLE_TENSORFLOW: Copy
// `-Xllvm -enable-experimental-cross-file-derivative-registration` into the
// .swiftinterface file.
if (A->getOption().matches(options::OPT_Xllvm) &&
StringRef(A->getValue()) ==
"-enable-experimental-cross-file-derivative-registration") {
A->render(Args, RenderedArgs);
continue;
}
if (A->getOption().hasFlag(options::ModuleInterfaceOption))
A->render(Args, RenderedArgs);
}
Expand Down Expand Up @@ -382,9 +373,13 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
if (Args.hasArg(OPT_enable_experimental_differentiable_programming))
Opts.EnableExperimentalDifferentiableProgramming = true;

// TODO: Ignore if enable-experimental-differentiable-programming is false
// TODO: Ignore differentiation-related flags if
// `enable-experimental-differentiable-programming` is false.
Opts.EnableExperimentalCrossFileDerivativeRegistration |=
Args.hasArg(OPT_enable_experimental_cross_file_derivative_registration);
Opts.EnableExperimentalForwardModeDifferentiation |=
Args.hasArg(OPT_enable_experimental_forward_mode_differentiation);

if (Args.hasArg(OPT_enable_experimental_quasiquotes))
Opts.EnableExperimentalQuasiquotes = true;

Expand Down
2 changes: 1 addition & 1 deletion lib/Sema/TypeCheckAttr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3797,7 +3797,7 @@ static bool typeCheckDerivativeAttr(ASTContext &Ctx, Decl *D,

// Reject different-file derivative registration.
// TODO(TF-1021): Lift same-file derivative registration restriction.
if (!EnableExperimentalCrossFileDerivativeRegistration &&
if (!ctx.LangOpts.EnableExperimentalCrossFileDerivativeRegistration &&
originalAFD->getParentSourceFile() != derivative->getParentSourceFile()) {
diags.diagnose(attr->getLocation(),
diag::derivative_attr_not_in_same_file_as_original);
Expand Down
4 changes: 2 additions & 2 deletions test/AutoDiff/downstream/cross_module_derivative_attr_e2e.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// RUN: %empty-directory(%t)
// RUN: %target-build-swift -working-directory %t -I%t -parse-as-library -emit-module -module-name module1 -emit-module-path %t/module1.swiftmodule -emit-library -static %S/Inputs/cross_module_derivative_attr_e2e/module1/module1.swift %S/Inputs/cross_module_derivative_attr_e2e/module1/module1_other_file.swift -Xllvm -enable-experimental-cross-file-derivative-registration -Xfrontend -validate-tbd-against-ir=none
// RUN: %target-build-swift -I%t -L%t %S/Inputs/cross_module_derivative_attr_e2e/main/main.swift -o %t/a.out -lm -lmodule1 -Xllvm -enable-experimental-cross-file-derivative-registration -Xfrontend -validate-tbd-against-ir=none
// RUN: %target-build-swift -working-directory %t -I%t -parse-as-library -emit-module -module-name module1 -emit-module-path %t/module1.swiftmodule -emit-library -static %S/Inputs/cross_module_derivative_attr_e2e/module1/module1.swift %S/Inputs/cross_module_derivative_attr_e2e/module1/module1_other_file.swift -Xfrontend -enable-experimental-cross-file-derivative-registration -Xfrontend -validate-tbd-against-ir=none
// RUN: %target-build-swift -I%t -L%t %S/Inputs/cross_module_derivative_attr_e2e/main/main.swift -o %t/a.out -lm -lmodule1 -Xfrontend -enable-experimental-cross-file-derivative-registration -Xfrontend -validate-tbd-against-ir=none
// RUN: %target-run %t/a.out
// REQUIRES: executable_test

Expand Down
6 changes: 3 additions & 3 deletions test/AutoDiff/downstream/derivative_registration_foreign/main.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
// RUN: %empty-directory(%t)
// RUN: %clang -c %S/Inputs/Foreign.c -fmodules -o %t/CForeign.o
// RUN: %target-swift-emit-silgen -Xllvm -enable-experimental-cross-file-derivative-registration -I %S/Inputs -I %t %s | %FileCheck %s --check-prefix=CHECK-SILGEN --check-prefix=CHECK
// RUN: %target-swift-emit-sil -Xllvm -enable-experimental-cross-file-derivative-registration -I %S/Inputs -I %t %s | %FileCheck %s --check-prefix=CHECK-SIL --check-prefix=CHECK
// RUN: %target-build-swift -Xllvm -enable-experimental-cross-file-derivative-registration -I %S/Inputs -I %t %s %t/CForeign.o
// RUN: %target-swift-emit-silgen -enable-experimental-cross-file-derivative-registration -I %S/Inputs -I %t %s | %FileCheck %s --check-prefix=CHECK-SILGEN --check-prefix=CHECK
// RUN: %target-swift-emit-sil -enable-experimental-cross-file-derivative-registration -I %S/Inputs -I %t %s | %FileCheck %s --check-prefix=CHECK-SIL --check-prefix=CHECK
// RUN: %target-build-swift -Xfrontend -enable-experimental-cross-file-derivative-registration -I %S/Inputs -I %t %s %t/CForeign.o
Copy link
Contributor

@rxwei rxwei Jan 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You do not need -Xfrontend when using the frontend. swiftc -enable-experimental... or swift -enable-experimental... should just work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing -Xfrontend from this RUN line doesn't work, I tried it.

$ llvm-project/llvm/utils/lit/lit.py -vvv build/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64/AutoDiff/downstream/derivative_registration_foreign
...
: 'RUN: at line 5';   xcrun --toolchain default --sdk '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk' /Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/bin/swiftc -target x86_64-apple-macosx10.9  -module-cache-path '/Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/swift-test-results/x86_64-apple-macosx10.9/clang-module-cache' -F '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks' -toolchain-stdlib-rpath -Xlinker -rpath -Xlinker '/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/Library/Frameworks'  -swift-version 4  -Xfrontend -ignore-module-source-info  -F '/Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/lib' -Xlinker -rpath -Xlinker '/Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/lib' -enable-experimental-cross-file-derivative-registration -I /Users/danielzheng/swift-dev/swift/test/AutoDiff/downstream/derivative_registration_foreign/Inputs -I /Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64/AutoDiff/downstream/derivative_registration_foreign/Output/main.swift.tmp /Users/danielzheng/swift-dev/swift/test/AutoDiff/downstream/derivative_registration_foreign/main.swift /Users/danielzheng/swift-dev/build/Ninja-ReleaseAssert/swift-macosx-x86_64/test-macosx-x86_64/AutoDiff/downstream/derivative_registration_foreign/Output/main.swift.tmp/CForeign.o
--
Exit Code: 1

Command Output (stderr):
--
/Users/danielzheng/swift-dev/swift/test/AutoDiff/downstream/derivative_registration_foreign/main.swift:28:2: error: derivative not in the same file as the original function
@derivative(of: cFunction)
 ^

This was unexpected to me too (swift -enable-experimental... doesn't always work), I noticed it for a while. Perhaps the options aren't registered properly in Options.td - we can investigate more later.

Copy link
Member

Choose a reason for hiding this comment

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

This is correct, the frontend flags are unsupported options, that is, they are within the purview of the compiler and may be changed at anytime (including having the behavior be entirely the opposite of the previous release). If you are using the driver (that is swiftc) you must specify -Xfrontend for the flag.

For this option, I believe that the current approach is desirable anyways as longer term we would like to remove the -enable-experimental-cross-file-derivative-registration flag and simply enable it unconditionally.

Copy link
Member

Choose a reason for hiding this comment

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

@rxwei I think that the confusion here may be related to the use of %target-build-swift which actually uses the driver and not the frontend.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I normally use %target-swift-frontend. Is there a reason we need to use %target-build-swift here, @dan-zheng ?

Copy link

Choose a reason for hiding this comment

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

%target-build-swift is closer to how normal swift compilation works, making the test more realistic so that it catches more bugs. I don't know exactly what the differences are, but I have observed that some symbols that are visible when you compile with %target-swift-frontend are actually hidden when you compile normally. Also, %target-build-swift respects swift_test_mode=optimize, so that it tests both -Onone and -O compilation.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks!


import CForeign

Expand Down