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

dan-zheng · 2020-01-21T21:17:10Z

Change -enable-experimental-cross-file-derivative-registration from an
-Xllvm flag to a -Xfrontend flag.

This makes the flag consistent with:

-enable-experimental-differentiable-programming
-enable-experimental-forward-mode-differentiation

Change `-enable-experimental-cross-file-derivative-registration` from an `-Xllvm` flag to a `-Xfrontend` flag. This makes the flag consistent with: - `-enable-experimental-differentiable-programming` - `-enable-experimental-forward-mode-differentiation`

rxwei · 2020-01-21T21:18:52Z

test/AutoDiff/downstream/derivative_registration_foreign/main.swift

-// 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


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

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.

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.

@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.

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

%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.

dan-zheng · 2020-01-21T21:26:04Z

@swift-ci Please test tensorflow

dan-zheng · 2020-01-21T21:47:50Z

@swift-ci Please clean test tensorflow linux

CodaFi · 2020-01-22T20:24:29Z

Is the intent for this to become a driver flag?

dan-zheng · 2020-01-22T20:29:32Z

Is the intent for this to become a driver flag?

Perhaps yes. We noticed that {swift,swiftc} -enable-experimental-cross-file-derivative-registration doesn't always work (#29339 (comment)), sometimes requiring -Xfrontend. I haven't investigated further yet.

To support {swift,swiftc} -enable-experimental-cross-file-derivative-registration, would you recommend making it a driver flag?

compnerd · 2020-01-23T03:01:31Z

To support {swift,swiftc} -enable-experimental-cross-file-derivative-registration, would you recommend making it a driver flag?

Given that we want to have this flag not be required in general I think that this being a frontend flag makes more sense (the user explicitly opts into this under the agreement that they are aware of what they are doing and realize that this behaviour may change).

rxwei · 2020-01-23T03:03:20Z

To support {swift,swiftc} -enable-experimental-cross-file-derivative-registration, would you recommend making it a driver flag?

Given that we want to have this flag not be required in general I think that this being a frontend flag makes more sense (the user explicitly opts into this under the agreement that they are aware of what they are doing and realize that this behaviour may change).

Makes sense.

compnerd · 2020-01-23T03:15:09Z

test/AutoDiff/downstream/derivative_registration_foreign/main.swift

-// 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


@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.

…ion`. Robust fix for validation-test/ParseableInterface/verify_all_overlays.py.

dan-zheng · 2020-01-23T19:35:33Z

@swift-ci Please test tensorflow

dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Jan 21, 2020

dan-zheng requested review from rxwei and marcrasi January 21, 2020 21:17

rxwei reviewed Jan 21, 2020

View reviewed changes

rxwei approved these changes Jan 21, 2020

View reviewed changes

compnerd approved these changes Jan 23, 2020

View reviewed changes

Mark relevant differentiable programming flags as `ModuleInterfaceOpt…

1fc4725

…ion`. Robust fix for validation-test/ParseableInterface/verify_all_overlays.py.

dan-zheng merged commit 677a671 into swiftlang:tensorflow Jan 23, 2020

dan-zheng deleted the retrodiff-frontend-flag branch January 23, 2020 21:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

dan-zheng commented Jan 21, 2020

Uh oh!

rxwei Jan 21, 2020 •

edited

Loading

Uh oh!

dan-zheng Jan 21, 2020

Uh oh!

compnerd Jan 23, 2020

Uh oh!

compnerd Jan 23, 2020

Uh oh!

rxwei Jan 23, 2020

Uh oh!

marcrasi Jan 23, 2020

Uh oh!

rxwei Jan 23, 2020

Uh oh!

dan-zheng commented Jan 21, 2020

Uh oh!

dan-zheng commented Jan 21, 2020

Uh oh!

CodaFi commented Jan 22, 2020

Uh oh!

dan-zheng commented Jan 22, 2020

Uh oh!

compnerd commented Jan 23, 2020

Uh oh!

rxwei commented Jan 23, 2020

Uh oh!

compnerd Jan 23, 2020

Uh oh!

dan-zheng commented Jan 23, 2020

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

dan-zheng commented Jan 21, 2020

Uh oh!

rxwei Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-zheng Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

compnerd Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

rxwei Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

marcrasi Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

rxwei Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

dan-zheng commented Jan 21, 2020

Uh oh!

dan-zheng commented Jan 21, 2020

Uh oh!

CodaFi commented Jan 22, 2020

Uh oh!

dan-zheng commented Jan 22, 2020

Uh oh!

compnerd commented Jan 23, 2020

Uh oh!

rxwei commented Jan 23, 2020

Uh oh!

compnerd Jan 23, 2020

Choose a reason for hiding this comment

Uh oh!

dan-zheng commented Jan 23, 2020

Uh oh!

Uh oh!

rxwei Jan 21, 2020 •

edited

Loading