Removed PH creation from Executor

Author: gcatron

include/glow/Runtime/RuntimeTypes.h

@@ -87,6 +87,10 @@ struct DAGNode {
   /// runtime.
   std::unique_ptr<RuntimeBundle> runtimeBundle;
 
+  /// Pointer to module the function came from. This is so the executor can
+  /// access the associated PHs for the function that are stored in the Module.
+  Module *module{nullptr};
+
   DeviceIDTy getNextDevice() {
     currentDeviceIdx++;
     return deviceIDs[currentDeviceIdx % deviceIDs.size()];

lib/Partitioner/Partitioner.cpp

@@ -460,6 +460,7 @@ void Partitioner::doPartitioning(Function *F, NodeToFunctionMap &mapping) {
   nodesDAGNodeTy nodes;
   DAGRoot->logicalDevices = {0};
   DAGRoot->name = F->getName();
+  DAGRoot->module = module_;
   DAGRoot->deviceIDs = {0};
   DAGNode *root = DAGRoot.get();
   llvm::DenseMap<Node *, Node *> currToNew;
@@ -580,6 +581,7 @@ llvm::Error Partitioner::Partition() {
       std::unique_ptr<DAGNode> DAG0 = llvm::make_unique<DAGNode>();
       DAG0->logicalDevices = {0};
       DAG0->name = F->getName();
+      DAG0->module = module_;
       std::unique_ptr<DAGNode> DAG1 = llvm::make_unique<DAGNode>();
       DAG1->logicalDevices = {0};
       DAG1->name = F->getName();

lib/Runtime/Executor/ThreadPoolExecutor.cpp

@@ -22,6 +22,8 @@
 #include <queue>
 #include <unordered_set>
 
+#include <glog/logging.h>
+
 namespace glow {
 namespace runtime {
 
@@ -59,7 +61,7 @@ ExecutionState::ExecutionState(RunIdentifierTy id, const DAGNode *root,
                                std::unique_ptr<ExecutionContext> resultContext,
                                ResultCBTy doneCb)
     : runId_(id), cb_(doneCb), resultCtx_(std::move(resultContext)),
-      inflightNodes_(0) {
+      inflightNodes_(0), module_(root->module) {
   // Create a queue for the breadth-first traversal through the graph.
   std::queue<const DAGNode *> bfsQueue;
 
@@ -101,8 +103,10 @@ ExecutionState::ExecutionState(RunIdentifierTy id, const DAGNode *root,
       const auto &symbolInfo = symbolPair.second;
 
       if (symbolInfo.symbolCategory == SymbolCategory::Placeholder) {
-        nodeInputPhBindings->allocate(
-            createOrGetPlaceholder(symbolName, &symbolInfo.type));
+        auto PH = module_->getPlaceholderByName(symbolName);
+        // If a PH name is provided it had to come from the Module originally.
+        DCHECK(PH) << "Placeholder: " << symbolName << " is not in the module";
+        nodeInputPhBindings->allocate(PH);
       }
     }
 
@@ -241,27 +245,6 @@ ExecutionContext *ExecutionState::getRawResultContextPtr() const {
   return resultCtx_.get();
 }
 
-Placeholder *ExecutionState::createOrGetPlaceholder(llvm::StringRef name,
-                                                    TypeRef type) {
-  auto it = intermediatePlaceholders_.find(name);
-  Placeholder *ph;
-
-  if (it != intermediatePlaceholders_.end()) {
-    // If the Placeholder already exists, return a pointer to it.
-    auto &storedPh = it->second;
-    ph = storedPh.get();
-  } else {
-    // If the Placeholder does not exist, create one, remember it, and return a
-    // pointer to it.
-    auto newPh =
-        llvm::make_unique<Placeholder>(name, type, /*isTrainable=*/false);
-    ph = newPh.get();
-    intermediatePlaceholders_.insert(std::make_pair(name, std::move(newPh)));
-  }
-
-  return ph;
-}
-
 void ThreadPoolExecutor::shutdown() {
   // Prevent more requests from being processed.
   shuttingDown_ = true;

lib/Runtime/Executor/ThreadPoolExecutor.h

@@ -111,10 +111,6 @@ class ExecutionState final {
   RunIdentifierTy getRunId() const { return runId_; }
 
 private:
-  /// Create a Placeholder with name \p name and type \p type and store it in
-  /// intermediatePlaceholders_. If a Placeholder already exists, return that.
-  Placeholder *createOrGetPlaceholder(llvm::StringRef name, TypeRef type);
-
   /// The run identifier for this execution of a DAG.
   RunIdentifierTy runId_;
   /// The callback that should be called when execution is done.
@@ -141,6 +137,10 @@ class ExecutionState final {
   /// Mutex used by bindings insertion functions to make sure only one thread
   /// writes to an ExecutionContext at a time.
   std::mutex bindingsMtx_;
+
+  /// Module for the network. This contains the PHs used by the functions in
+  /// this network.
+  Module *module_{nullptr};
 };
 
 /// This implementation of the Executor interface uses a thread pool to

tests/unittests/ExecutorTest.cpp

@@ -201,16 +201,20 @@ class ExecutorTest final {
 public:
   /// Constructor.
   ExecutorTest(const std::shared_ptr<Executor> &executor,
-               std::unique_ptr<DAGNode> root, std::unique_ptr<Type> type,
-               DAGNodeNameMapTy nodes, PlaceholderNameMapTy placeholders,
+               std::unique_ptr<DAGNode> root, std::unique_ptr<Module> module,
+               std::unique_ptr<Type> type, DAGNodeNameMapTy nodes,
+               PlaceholderNameMapTy placeholders,
                std::unique_ptr<ExecutionContext> inputContext,
                std::unique_ptr<ExecutionContext> outputContext,
                RunIdentifierTy runId, bool expectSuccess)
-      : executor_(executor), root_(std::move(root)), type_(std::move(type)),
-        nodes_(std::move(nodes)), placeholders_(std::move(placeholders)),
+      : executor_(executor), root_(std::move(root)), module_(std::move(module)),
+        type_(std::move(type)), nodes_(std::move(nodes)),
+        placeholders_(std::move(placeholders)),
         inputContext_(std::move(inputContext)),
         outputContext_(std::move(outputContext)), runId_(runId),
-        expectSuccess_(expectSuccess), testRun_(false) {}
+        expectSuccess_(expectSuccess), testRun_(false) {
+    root_->module = module_.get();
+  }
 
   /// Run the test.
   bool run() {
@@ -260,6 +264,8 @@ class ExecutorTest final {
   std::shared_ptr<Executor> executor_;
   /// The root node of the DAG being tested.
   std::unique_ptr<DAGNode> root_;
+  /// The Module containing the PHs.
+  std::unique_ptr<Module> module_;
   /// The Type for all of the Placeholders that will be used during execution.
   std::unique_ptr<Type> type_;
   /// All nodes in the DAG.
@@ -295,7 +301,8 @@ class ExecutorTestBuilder final {
   /// between ExecutionContexts correctly.
   ExecutorTestBuilder(const std::shared_ptr<Executor> &executor,
                       const DeviceManagerMapTy &deviceManagers)
-      : executor_(executor), root_(llvm::make_unique<DAGNode>()),
+      : executor_(executor), module_(llvm::make_unique<Module>()),
+        root_(llvm::make_unique<DAGNode>()),
         bindings_(llvm::make_unique<PlaceholderBindings>()),
         type_(
             std::unique_ptr<Type>(new Type(ElemKind::FloatTy, {32, 64, 128}))),
@@ -465,16 +472,16 @@ class ExecutorTestBuilder final {
       insertSymbolIntoPlaceholderBindings(
           symbol, outputContext->getPlaceholderBindings());
     }
-
     // Create the test object.
-    ExecutorTest test(executor_, std::move(root_), std::move(type_),
-                      std::move(nodes_), std::move(placeholders_),
-                      std::move(inputContext), std::move(outputContext), runId_,
-                      success_);
+    ExecutorTest test(executor_, std::move(root_), std::move(module_),
+                      std::move(type_), std::move(nodes_),
+                      std::move(placeholders_), std::move(inputContext),
+                      std::move(outputContext), runId_, success_);
 
     // Reset builder state to allow a new test to be constructed with this
     // instance.
     root_ = llvm::make_unique<DAGNode>();
+    module_ = llvm::make_unique<Module>();
     bindings_->clear();
     type_ = std::unique_ptr<Type>(new Type(ElemKind::FloatTy, {1, 2, 2}));
     nodes_.clear();
@@ -537,28 +544,27 @@ class ExecutorTestBuilder final {
   /// mapped for the test being created, reuse the existing value.
   void insertSymbolIntoPlaceholderBindings(llvm::StringRef name,
                                            PlaceholderBindings *bindings) {
-    auto it = placeholders_.find(name);
+    auto ph = module_->getPlaceholderByName(name);
 
-    if (it == placeholders_.end()) {
+    if (!ph) {
       // This is a new symbol. Create a Placeholder and an initialize and new
       // Tensor for it.
-      auto placeholder = llvm::make_unique<Placeholder>(name, type_.get(),
-                                                        /*trainable=*/false);
-      auto *tensor = bindings_->allocate(placeholder.get());
+      auto placeholder = module_->createPlaceholder(type_.get(), name, false);
+      auto *tensor = bindings_->allocate(placeholder);
       tensor->init(Tensor::InitKind::Xavier, 1.0, rng_);
-      bindings->insert(placeholder.get(), tensor->clone());
-      placeholders_[name] = std::move(placeholder);
+      bindings->insert(placeholder, tensor->clone());
     } else {
       // This is a symbol that already has an associated Placeholder and Tensor.
       // Copy that Tensor.
-      auto *placeholder = (it->second).get();
-      const auto *tensor = bindings_->get(placeholder);
-      bindings->insert(placeholder, tensor->clone());
+      const auto *tensor = bindings_->get(ph);
+      bindings->insert(ph, tensor->clone());
     }
   }
 
   /// The Executor being tested.
   std::shared_ptr<Executor> executor_;
+  /// Module for holding PHs
+  std::unique_ptr<Module> module_;
   /// The root of the DAG being constructed.
   std::unique_ptr<DAGNode> root_;
   /// This PlaceholderBindings holds all created and initialized Placeholders