Wrapped TF C APIs as runtime functions, so that IRGen can generate calls to them #19555

mhong · 2018-09-26T04:19:41Z

This patch "dissolved" the call to the packaged experimental C API TF_RunConstOp() into a set of finer-grained C API calls.

An alternative to the runtime function approach is to use Clang importer to call
the C APIs. Clang importer allows the C APIs to become available as SIL
functions with the same names, and thus can be found and called in IRGen via
silModule.findFunction() (see
https://github.com/apple/swift/blob/ad7def2c6bffd95d62e5e665c9faed0f8dac49f5/lib/IRGen/IRGenSIL.cpp#L1956-L1957). The concerns with this approach are:

There are no precedents in IRGen that calls C APIs via this clang importer
route. Having IRGen depend on clang importer may not be desirable.
Some C APIs cannot be found via clang importer (e.g. TF_NewStatus() can be
found, but TFE_NewOp() cannot). As a work-around, these C APIs must be called in
the user module code first, before IRGen over the user module can locate them as
SIL functions. This feels brittle.
When passing objects between compiler runtime entry points (swift functions)
such as @_silgen_name("_swift_tfc_GetGlobalEagerContext") and the C APIs, IRGen
have to issue bitcast to convert between a real struct type (e.g. TFE_Context*)
and void* (i8* in LLVM type) as used in the entry points.

In contrast, the wrapped runtime functions consistently use void* in their
interfaces, so that IRGen need not do such bitcasts.

One downside with the runtime function approach is that we need to wrap ~20 TF C
APIs in the new files TensorFlow.{h,cpp}. This is however mostly a one-time eng cost.

…lls to them. This patch "disolved" the call to the packaged experimental C API TF_RunConstOp() into a set of finer-grained C API calls. An alternative to the runtime function approach is to use Clang importer to call the C APIs. Clang importer allows the C APIs to become available as SIL functions with the same names, and thus can be found and called in IRGen via silModule.findFunction() (see https://github.com/apple/swift/blob/ad7def2c6bffd95d62e5e665c9faed0f8dac49f5/lib/IRGen/IRGenSIL.cpp#L1956-L1957). The concerns with this approach are: 1. There are no precedents in IRGen that calls C APIs via this clang importer route. Having IRGen depend on clang importer may not be desirable. 2. Some C APIs cannot be found via clang importer (e.g. TF_NewStatus() can be found, but TFE_NewOp() cannot). As a work-around, these C APIs must be called in the user module code first, before IRGen over the user module can locate them as SIL functions. This feels brittle. 3. When passing objects between compiler runtime entry points (swift functions) such as @_silgen_name("_swift_tfc_GetGlobalEagerContext") and the C APIs, IRGen have to issue bitcast to convert between a real struct type (e.g. TFE_Context*) and void* (i8* in LLVM type) as used in the entry points. In contrast, the wrapped runtime functions consistently use void* in their interfaces, so that IRGen need not do such bitcasts. One downside with the runtime function approach is that we need to wrap ~20 TF C APIs in the new files TensorFlow.{h,cpp}. This is however mostly a one-time eng cost.

mhong · 2018-09-26T04:19:55Z

@swift-ci please test tensorflow

lattner · 2018-09-26T04:29:49Z

lib/IRGen/IRGenSIL.cpp

+  auto *TFNewStatusFn = IGM.getTF_NewStatusFn();
+  auto status = Builder.CreateCall(TFNewStatusFn, {});
+
+  if (opInfo.getOperationName() != "Const") {


Special casing the Const op is just a temporary thing for your incremental development, right?

Yup! There was a TODO above.

lattner · 2018-09-26T04:30:14Z

lib/IRGen/IRGenSIL.cpp

+  auto opNameValAddr = llvm::ConstantExpr::getInBoundsGetElementPtr(
+      global->getValueType(), global, indices);
+  return opNameValAddr;
+}


This logic all LGTM,

lattner · 2018-09-26T04:31:50Z

include/swift/Runtime/TensorFlow.h

+//===----------------------------------------------------------------------===//
+
+#include "swift/Runtime/Config.h"
+#include <stdint.h>


<cstdint> please.

lattner · 2018-09-26T04:33:45Z

include/swift/Runtime/RuntimeFunctions.def

+         ARGS(Int32Ty),
+         ATTRS(NoUnwind))
+
+FUNCTION(TF_DeleteTensor, swift_tfc_TF_DeleteTensor, C_CC,


Out of curiosity, why do you need these wrappers? Why can't you just declare TF_DeleteTensor here with the void* flavored signature? It should all work out due to the behavior of getRuntimeFunction calling LLVM's getOrCreateFunction API that returns the function or does a bitcast if an incompatible signature is already present.

Things like _swift_tfc_CheckOk that are not trivial wrappers definitely will require entries here, I'm just asking about the trivial wrappers like the TF_DeleteTensor case.

I'll try this and report back.

Yes I'm able to remove the decls and impls in TensorFlow.{h,cpp}. The entries in RuntimeFunctions.def remain, so that we can call them in IRGen (e.g. via IGM.getTF_DeleteTensorFn()). Thanks!

lattner · 2018-09-26T04:35:23Z

lib/IRGen/IRGenSIL.cpp

-      IGM.getAddrOfSILFunction(getContextSilFn, NotForDefinition);
-  assert(getContextFn);
+  llvm::Function *getContextFn =
+      findFunction("_swift_tfc_GetGlobalEagerContext", silModule);


Do you plan to move this one to runtimefunctions as well?

How can I change those swift functions to be callable as runtime functions (e.g. i need to access swift objects like _ExecutionContext.global.eagerContext somehow)?

If we can do that, we may not need #19556, which would be nice.

Just make sure they have a convention(c) calling convention and public linkage and you can call them as C functions!

Simple change:
Replace @_silgen_name with @_cdecl and you'll get C-convention functions.

Thanks. Converted some entry points to @_cdecl, and added runtime function entries for them.

For the remaining ones, I'll convert them once we use the new AnyTensorHandle.

lattner

Awesome, thank you for going down this path. I agree it is a pain given the number of runtime functions we have to deal with, but I think it is a good thing to align with the existing design of IRGen rather than paving new roads.

1. Removed runtime func impls, when we can us the C API ones. 2. Changed some compiler RT entry points to using @_cdecl, so that they can be called in IRGen directly (instead of going through clang importer and silgen names).

mhong · 2018-09-26T06:12:24Z

@swift-ci please clean test tensorflow

rxwei · 2018-09-26T06:42:13Z

stdlib/public/TensorFlow/CompilerRuntime.swift

@@ -1183,3 +1178,8 @@ public func _CreateTensorHandleFromCTensorHandle(
  return TensorHandle<Float>(owning: ownedCHandle)
 }

+@inlinable
+@_cdecl("_swift_tfc_CheckOk")
+public func _CheckOk(_ s: CTFStatus) {


All our previous existing exported runtime functions have prefix “_TFC” in the Swift name. These should be updated for consistency. For example, this function should be “_TFCCheckOK”.

rxwei · 2018-09-26T06:43:10Z

stdlib/public/TensorFlow/CompilerRuntime.swift

-
-@inlinable
-@_silgen_name("_swift_tfc_GetGlobalEagerContext")
+@_cdecl("_swift_tfc_GetGlobalEagerContext")
 public func _GetGlobalEagerContext() -> CTFEContext {


I don’t think these have to be public. @usableFromInline should work fine. Have you tried it?

Yes this works for the @_cdecl functions. Changed.

rxwei · 2018-09-26T06:47:22Z

lib/IRGen/IRGenSIL.cpp

+    return fn;
+  }
+
+  void checkOk(llvm::Value *status) {


CheckOk is a really generic name, but it’s tied to TFC. After prefixes for runtime functions are added, this function should be renamed accordingly to prevent confusion with the rest of the IRGen code base.

Added the TFC prefix.

rxwei · 2018-09-26T06:49:27Z

lib/IRGen/IRGenSIL.cpp

+  // SWIFT_ENABLE_TENSORFLOW
+  // Returns the LLVM function with `funcName`. It must exist.
+  llvm::Function *findFunction(StringRef funcName, SILModule &silModule) {
+    LLVM_DEBUG(llvm::dbgs() << "IRGen for " << funcName << "().\n");


This debug comment is confusing. It looks like it’s print the status that the compiler is running IRGen on a function, but it is not. It’s looking up a function.

Changed to "IRGen for calling ". I expect findFunction() to go away once we have AnyTensorHandle, since all compiler runtime entry points can be defined as C functions at that time.

That's great

mhong · 2018-09-26T14:01:15Z

@swift-ci please clean test tensorflow

swift/stdlib/public/runtime/CMakeLists.txt, because CI run (e.g. https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-TensorFlow-Linux/859/console) reported: ``` CMake Error at stdlib/public/runtime/CMakeLists.txt:30 (include_directories): include_directories given empty-string as include directory. ```

mhong · 2018-09-26T17:33:28Z

@swift-ci please clean test tensorflow

lattner · 2018-09-26T18:51:09Z

include/swift/Runtime/RuntimeFunctions.def

+// - Naming convention:
+//   - TF_XXX functions correspond to TF C APIs
+//   - TFE_XXX functions correspond to TF eager C APIs
+//   - TFC_XXX functions are C functions defined in the compiler runtime


lattner · 2018-09-26T18:51:54Z

stdlib/public/runtime/CMakeLists.txt

@@ -27,6 +27,7 @@ list(APPEND swift_runtime_compile_flags
     "-D__SWIFT_CURRENT_DYLIB=swiftCore")

 # SWIFT_ENABLE_TENSORFLOW
+find_package(TensorFlow REQUIRED)


What does this do?

Update: I moved code in the new files TensorFlow.{h,cpp} to the CTensorFlow
module (ctensorflow_init.{h,cpp}), so that the stdlib/public/runtime/ need not
depend on the TF library. As such I reverted the changes to the CMakefile here.

More context for the original plan:
find_package() uses the swift/cmake/modules/FindTensorFlow.cmake script to define the TF_INCLUDE_DIR variable, which the next line uses, so that the new runtime source file swift/stdlib/public/runtime/TensorFlow.cpp can find header files like "tensorflow/c/c_api.h".

https://cmake.org/cmake/help/v3.0/command/find_package.html has more context.

lattner

Patch looks great, thanks!

rxwei · 2018-09-26T19:12:14Z

stdlib/public/TensorFlow/Utilities.swift

@@ -70,7 +70,7 @@ func checkOk(_ s: CTFStatus?, file: StaticString = #file, line: UInt = #line) {
 typealias CTFSession = OpaquePointer

 /// The `TF_Status *` type.
-typealias CTFStatus = OpaquePointer
+public typealias CTFStatus = OpaquePointer


Since _TFCCheckOk is @usableFromInline now, this typealias can also be @usableFromInline.

module (ctensorflow_init.{h,cpp}), so that the stdlib/public/runtime/ need not depend on the TF library. Also addresed more feedback.

mhong · 2018-09-26T19:52:27Z

@swift-ci please clean test tensorflow

mhong · 2018-09-26T21:53:02Z

Linux tests passed:
https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-TensorFlow-Linux/861/console
https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-TensorFlow-Linux-gpu/113/

Mac test hit a network error (https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-TensorFlow-macOS/90/console):

> git fetch --tags --progress https://github.com/apple/swift.git +refs/pull/*:refs/remotes/origin/pr/*
ERROR: Error fetching remote repo 'origin'

I'll retry it.

mhong · 2018-09-26T21:53:06Z

@swift-ci Please test tensorflow macOS

bgogul

Nice!

mhong · 2018-09-26T23:29:48Z

Mac test finished: https://ci-external.swift.org/view/Pull%20Request/job/swift-PR-TensorFlow-macOS/92/

Thanks all for the review!

mhong requested review from rxwei, marcrasi, lattner and bgogul September 26, 2018 04:20

Reverted some compiler rt changes.

f44e862

lattner reviewed Sep 26, 2018

View reviewed changes

lattner approved these changes Sep 26, 2018

View reviewed changes

Addressed feedback:

3d7fe3e

1. Removed runtime func impls, when we can us the C API ones. 2. Changed some compiler RT entry points to using @_cdecl, so that they can be called in IRGen directly (instead of going through clang importer and silgen names).

rxwei reviewed Sep 26, 2018

View reviewed changes

mhong mentioned this pull request Sep 26, 2018

Relaxed the checks in SILDeserializer::readSILFunctionChecked() #19556

Closed

Mingsheng Hong added 2 commits September 26, 2018 10:22

Addressed comments from @rxwei

5d374fb

lattner reviewed Sep 26, 2018

View reviewed changes

lattner approved these changes Sep 26, 2018

View reviewed changes

rxwei reviewed Sep 26, 2018

View reviewed changes

rxwei approved these changes Sep 26, 2018

View reviewed changes

Moved code in TensorFlow.{h,cpp} to the CTensorFLow

a40bd00

module (ctensorflow_init.{h,cpp}), so that the stdlib/public/runtime/ need not depend on the TF library. Also addresed more feedback.

bgogul approved these changes Sep 26, 2018

View reviewed changes

mhong merged commit 1c5ea9d into swiftlang:tensorflow Sep 26, 2018

mhong deleted the irgen_4_runtime_funcs branch September 26, 2018 23:30

Wrapped TF C APIs as runtime functions, so that IRGen can generate calls to them #19555

Wrapped TF C APIs as runtime functions, so that IRGen can generate calls to them #19555

Uh oh!

Conversation

mhong commented Sep 26, 2018

Uh oh!

mhong commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhong Sep 26, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lattner left a comment

Choose a reason for hiding this comment

Uh oh!

mhong commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhong commented Sep 26, 2018

Uh oh!

mhong commented Sep 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lattner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhong Sep 26, 2018 •

edited

Loading