Tensorflow merge #29964
Merged
Tensorflow merge #29964
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…f9c9c8e10701a0
Clean up `_add_swift_host_library_single`
…2f9f353f5a6ccd [ownership] Add copy constructor/assignment operator for BorrowScopeOperand.
This moves the swift-lang library out of the tools/SourceKit directory and into the stdlib directory. As it stands, the library required that the standard library and the SDK overlay was being built. This is implicitly done when building the stdlib subdirectory. Furthermore, it had an odd tie between the flags for Swift and SourceKit which now have the logic separated. This clearly delineates the host and target components in the repository.
[Incremental] Reorganization of SourceFileDepGraph building and mocking.
This reverts commit f071cf1. With ASAN fixes.
This patch implements movable guaranteed scopes in ossa. This pattern is currently not generated anywhere in the compiler, but my hope is to begin emitting these in SemanticARCOpts. The idea is that these model true phi nodes and thus can be used to fuse multiple guaranteed scopes into one using br instructions. This is treated similarly to how owned instructions are forwarded through /all/ terminators. This will enable us to use the SILSSAUpdater with guaranteed arguments as well as enable the expression of sets of borrow scopes that minimally jointly-dominate a guaranteed argument. This will enable us to express +0 merge points like the following: ``` bb1: %0a = begin_borrow %0 : $Klass br bb3(%0a : $Klass) bb2: %1a = load_borrow %1 : $*Klass br bb3(%1a : $Klass) bb3(%2 : @guaranteed $Klass) ... end_borrow %2 : $Klass ``` I describe below what the semantics of guaranteed block arguments were previously, what they are now, and a little bit of interesting things from a semantic perspective around implicit sub-scope users. Before this patch in ossa, guaranteed block arguments had two different sets of semantics: 1. Given a checked_cast_br or a switch_enum, the guaranteed block argument was treated like a forwarding instruction. As such, the guaranteed argument's did not require an end_borrow and its uses were validated as part of the use list of the switch_enum/checked_cast_br operand's borrow introducer. It also was not classified as a BorrowScopeValueIntroducer since it was not introducing a new scope. 2. Given any other predecessor terminator, we treated the guaranteed argument as a complete sub-scope of its incoming values. Thus we required the guaranteed argument to have its lifetime eneded by an end_borrow and that all incoming values of the guaranteed argument to come from a borrow introducer whose set of jointly post-dominating end_borrows also jointly post-dominates the set of end_borrows associated with the guaranteed argument itself. Consider the following example: ``` bb0: %1 = begin_borrow %foo : $Foo // (1) %2 = begin_borrow %foo2 : $Foo2 // (2) cond_br ..., bb1, bb2 bb1: br bb3(%1 : $Foo) bb2: br bb3(%2 : $Foo) bb3(%3 : @guaranteed $Foo) ... end_borrow %3 : $Foo // (3) end_borrow %2 : $Foo // (4) end_borrow %1 : $Foo // (5) ... ``` Notice how due to SSA, (1) and (2) must dominate (4) and (5) and thus must dominate bb3, preventing the borrows from existing within bb1, bb2. This dominance property is actively harmful to expressivity in SIL since it means that guaranteed arguments can not be used to express (without contortion) sil code patterns where an argument is jointly-dominated by a minimal set of guaranteed incoming values. For instance, consider the following SIL example: ``` bb0: cond_br ..., bb1, bb2 bb1: %0 = load [copy] %globalAddr : $Foo br bb3(%0 : $Foo) bb2: %1 = copy_value %guaranteedFunctionArg : $Foo br bb3(%1 : $Foo): bb3(%2 : @owned $Foo): apply %useFoo(%2) destroy_value %2 : $Foo ``` As a quick proof: Assume the previous rules for guaranteed arguments. Then to promote the load [copy] -> load_borrow and the copy_value to a begin_borrow, we would need to place an end_borrow in bb3. But neither bb1 or bb2 dominates bb3, so we would violate SSA dominance rules. To enable SIL to express this pattern, we introduce a third rule for terminator in ossa that applies only to branch insts. All other branches that obeyed the previous rules (cond_br), still follow the old rule. This is not on purpose, I am just being incremental and changing things as I need to. Specifically, guaranteed arguments whose incoming values are defined by branch instructions now act as a move on guaranteed values. The intuition here is that these arguments are acting as true phis in an SSA sense and thus are just new names for the incoming values. This implies since it is just a new name (not a semantic change) that the guaranteed incoming value's guaranteed scopes should be fused into one scope. The natural way to model this is by treating branch insts as consuming guaranteed values. This then lets us express the example above without using copies as follows: ``` bb0: cond_br ..., bb1, bb2 bb1: %0 = load_borrow %globalAddr : $Foo br bb3(%0 : $Foo) // consumes %0 and acts as %0's end_borrow. bb2: // We need to introduce a new begin_borrow here since function // arguments are required to never be consumed. %1 = begin_borrow %guaranteedFunctionArg : $Foo br bb3(%1 : $Foo) // consumes %1 and acts as %1's end_borrow // %2 continues the guaranteed scope of %0, %1. This time fused with one name. bb3(%2 : @guaranteed $Foo): apply %useFoo(%2) // End the lifetime of %2 (which implicitly ends the lifetime of %0, %1). end_borrow %2 : $Foo ... ``` The main complication for users is that now when attempting to discover the set of implicit users on an owned or guaranteed value caused by their usage as an argument of a borrow introducer like begin_borrow. For those who are unaware, a begin_borrow places an implicit requirement on its parent value that the parent value is alive for the entire part of the CFG where this begin_borrow is live. Previously, one could just look for the end_borrows of the begin_borrow. Now one must additionally look for consuming branch insts. This is because the original value that is being borrowed from must be alive over the entire web of guaranteed values. That is the entire web of guaranteed values act as a liveness requirement on the begin_borrow's operand. The way this is implemented is given a use that we are validating, if the use is a BorrowScopeOperand (1), we see if the borrow scope operand consumes the given guaranteed scope and forwards it into a borrow scope introducer. If so, we add the list of consuming uses of the borrow scope introducer to the worklist to visit and then iterate. In order to avoid working with cycles, for now, the ownership verifier bans liveness requiring uses that have cycles in them. This still allows us to have loop carried guaranteed values. (1) A BorrowScopeOperand is a concept that represents an operand to a SIL instruction that begins a guaranteed scope of some sort. All BorrowScopeOperand are thus at a minimum able to compute a compile time the static region in which they implicitly use their operands. NOTE: We do not require the scope to be represented as a SILValue in the same function. We achieve some nice benefit by introducing this. Specifically: 1. We can optimize the pattern I mentioned above. This is a common pattern in many frameworks that want to return a default object if a computation fails (with the default object usually being some sort of global or static var). This will let us optimize that case when the global is a let global. 2. The SSA Updater can now be used with guaranteed values without needing to introduce extra copies. This will enable predictable mem opts to introduce less copies and for semantic arc opts to optimize the remaining copies that PMO exposes but does not insert itself. rdar://56720519
The main change is that we do not eliminate end_borrows when propagating guaranteed phis. This is because phis now forward guaranteed ownership like owned ownership and since we only eliminate these arguments if all incomign values to the argument is the same (providing dominance).
[Stdlib] Eagerly realize EmptyDictionarySingleton and EmptySetSingleton.
…uses a use-after-free. A "copy_addr [take] %src to [initialization] %alloc_stack" is replaced by a "destroy_addr %src" if the alloc_stack is otherwise dead. This is okay as long as the "moved" object is kept alive otherwise. This can break if a retain of %src is moved after the copy_addr. It cannot happen with OSSA. So as soon as we have OSSA, we can remove the check again. rdar://problem/59509229
[Diagnostics] Cleanup logic related to argument mismatch
SILCombine: fix a miscompile in the alloc_stack optimization which causes a use-after-free.
[CSApply] Always use `String` type for ObjC interop key path
Regardless of any flags, the stdlib will have its generic metadata prespecialized. Temporarily reintroduced the flag to enable the feature flag while preserving the flag to disable it and changed the default back to off for the moment.
…a8a6146cb7d446 Revert "Revert "[basic] Add a simple vector backed 2 stage multi map.""
…c2ff7e4b73271a [gardening] Update a bit-rotted header.
[IRGen] NFC: silence `llvm::MaybeAlign` warnings.
Revert "Revert "swift-lang: shuffle the source directory structure (N…
Applies #29958 and applies the same logic to tensorflow.py.
|
Let's see if CI is still down. |
dan-zheng
approved these changes
Feb 20, 2020
|
@swift-ci Please clean test tensorflow |
Propagate `--enable-tensorflow` from build-script to CMake as `-DSWIFT_ENABLE_TENSORFLOW:BOOL=TRUE`. Partially unreverts 73ebd5b. This is significant for lit tests, so that `test/lit.site.cfg.in` adds `tensorflow` as an available feature.
|
I disabled |
|
@swift-ci Please test tensorflow |
|
@swift-ci Please test tensorflow macOS |
dan-zheng
reviewed
Feb 21, 2020
| @@ -2377,6 +2407,8 @@ skip-test-playgroundsupport | |||
| mixin-preset= | |||
| tensorflow_osx | |||
| mixin_codesigning | |||
| tensorflow-swift-apis | |||
| pythonkit | |||
There was a problem hiding this comment.
macOS toolchain builds (via utils/build-toolchain-tensorflow --pkg) currently fail while building pythonkit:
...
+ /usr/local/bin/cmake --build /Users/swiftninjas/s4tf/build/buildbot_osx/pythonkit-macosx-x86_64
[1/1][100%][0.341s] Linking Swift shared library PythonKit/libPythonKit.dylib
FAILED: PythonKit/libPythonKit.dylib PythonKit/CMakeFiles/PythonKit.dir/NumpyConversion.swift.o PythonKit/CMakeFiles/PythonKit.dir/Python.swift.o PythonKit/CMakeFiles/PythonKit.dir/PythonLibrary+Symbols.swift.o PythonKit/CMakeFiles/PythonKit.dir/PythonLibrary.swift.o PythonKit/PythonKit.swiftmodule
: && /Users/swiftninjas/s4tf/swift/swift-nightly-install/Library/Developer/Toolchains/swift-tensorflow-DEVELOPMENT-2020-02-20-a.xctoolchain/usr/bin/swiftc -output-file-map PythonKit/CMakeFiles/PythonKit.dir/output-file-map.json -incremental -num-threads 12 -emit-library -o PythonKit/libPythonKit.dylib -module-name PythonKit -module-link-name PythonKit -emit-module -emit-module-path PythonKit/PythonKit.swiftmodule -emit-dependencies -DPythonKit_EXPORTS /Users/swiftninjas/s4tf/PythonKit/PythonKit/NumpyConversion.swift /Users/swiftninjas/s4tf/PythonKit/PythonKit/Python.swift /Users/swiftninjas/s4tf/PythonKit/PythonKit/PythonLibrary+Symbols.swift /Users/swiftninjas/s4tf/PythonKit/PythonKit/PythonLibrary.swift && :
/Users/swiftninjas/s4tf/PythonKit/PythonKit/PythonLibrary.swift:18:8: error: cannot load underlying module for 'Darwin'
import Darwin
^
<unknown>:0: note: did you forget to set an SDK using -sdk or SDKROOT?
<unknown>:0: note: use "xcrun swiftc" to select the default macOS SDK installed with Xcode
ninja: build stopped: subcommand failed.
Building the standard library for: swift-stdlib-macosx-x86_64
Running Swift tests for: check-swift-all-macosx-x86_64@compnerd: do you have any idea why this occurs?
I'm using CMake 3.15.4 on macOS, upgrading to 3.16.4 now in case that resolves the issue.
There was a problem hiding this comment.
I tried some things (started clean builds, etc) and can't reproduce the issue above anymore.
Now, macOS toolchain builds are stuck at building tensorflow. ctrl t suggests that build-script is stuck at running yes:
Preconfigured Bazel build configs. You can use any of the below by adding "--config=<>" to your build command. See .bazelrc for more details.
--config=mkl # Build with MKL support.
--config=monolithic # Config for mostly static monolithic build.
--config=ngraph # Build with Intel nGraph support.
--config=numa # Build with NUMA support.
--config=dynamic_kernels # (Experimental) Build kernels into separate shared objects.
--config=v2 # Build TensorFlow 2.x instead of 1.x.
Preconfigured Bazel build configs to DISABLE default on features:
--config=noaws # Disable AWS S3 filesystem support.
--config=nogcp # Disable GCP support.
--config=nohdfs # Disable HDFS support.
--config=nonccl # Disable NVIDIA NCCL support.
Configuration finished
load: 3.02 cmd: yes 94555 running 477.87u 0.39s
load: 3.02 cmd: yes 94555 running 478.69u 0.39s
load: 3.02 cmd: yes 94555 running 478.92u 0.39s
load: 3.02 cmd: yes 94555 running 479.24u 0.39s
load: 3.02 cmd: yes 94555 running 479.43u 0.39s
load: 3.02 cmd: yes 94555 running 479.68u 0.39s
load: 3.02 cmd: yes 94555 running 479.80u 0.39s
load: 3.02 cmd: yes 94555 running 479.90u 0.39sThis seems likely to be a bug.
I forgot to add this in #29982, sorry.
Disable just like `stability-stdlib-abi-with-asserts.swift`. Same explanatory comment.
|
@swift-ci Please clean test tensorflow |
1 similar comment
|
@swift-ci Please clean test tensorflow |
Update checkout to `swift-DEVELOPMENT-SNAPSHOT-2020-02-20-a`. This is important for macOS toolchain builds. Maybe other things too.
|
The macOS CI machine encounters three test failures: The last two failures seem unexpected: I wonder if the issue is that |
Fix Python formatting and undefined references.
Fix error introduced in #29997. Todo: minimize utils/build-script diff with master branch.
|
@swift-ci Please clean test tensorflow |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Merge master into the S4TF tensorflow branch.