Revert: [Placeholder] Add support for Placeholders in the execution engine #1586

This commit reverts the parts of the PR #1586 that pass the placeholder
variables in the run command of the execution engine. At first I thought
that this would allow to eliminate the variables from the high-level
parts of the compilation, but I realized that we need to start with
fixing all of the backends and add support to the compiled function.

Author: nadavrot

include/glow/Backends/CompiledFunction.h

@@ -16,23 +16,16 @@
 #ifndef GLOW_BACKENDS_COMPILEDFUNCTION_H
 #define GLOW_BACKENDS_COMPILEDFUNCTION_H
 
-#include "llvm/ADT/ArrayRef.h"
-
 namespace glow {
 
-class Placeholder;
-class Tensor;
-
 /// Interface for executing a compiled function.
 class CompiledFunction {
 public:
   /// Dtor.
   virtual ~CompiledFunction() = default;
 
-  /// Execute the network. The backing tensors in \p tensors are mapped to the
-  /// placeholders of \p placeholders for the invocation of the program.
-  virtual void execute(llvm::ArrayRef<Placeholder *> placeholders,
-                       llvm::ArrayRef<Tensor *> tensors) = 0;
+  /// Execute the network.
+  virtual void execute() = 0;
 };
 
 } // end namespace glow

include/glow/ExecutionEngine/ExecutionEngine.h

@@ -80,19 +80,19 @@ class ExecutionEngine final {
 
   /// Runs a single execution of the function. This method updates the variables
   /// in \p nodes with the tensor content values \p inputs.
-  void run(llvm::ArrayRef<Storage *> vars, llvm::ArrayRef<Tensor *> inputs);
+  void run(llvm::ArrayRef<Variable *> vars, llvm::ArrayRef<Tensor *> inputs);
 
   /// Runs \p iterations iterations of the function. The method updates a local
   /// counter and future invocations of this method continue running iterations
   /// of the batch at the next available slice.
   /// The method updates the variables in \p vars with the tensors \p inputs.
-  void runBatch(size_t iterations, llvm::ArrayRef<Storage *> vars,
+  void runBatch(size_t iterations, llvm::ArrayRef<Variable *> vars,
                 llvm::ArrayRef<Tensor *> inputs);
 
 private:
   /// Update the inputs for all variables \p vars with data from the inputs \p
   /// inputs at offset \p sampleIdx. Then perform a run of the network.
-  void updateInputsAndRunNetwork(llvm::ArrayRef<Storage *> vars,
+  void updateInputsAndRunNetwork(llvm::ArrayRef<Variable *> vars,
                                  llvm::ArrayRef<Tensor *> inputs,
                                  size_t sampleIdx);
 

lib/Backends/CPU/CPUFunction.cpp

@@ -26,8 +26,7 @@ CPUFunction::CPUFunction(std::unique_ptr<llvm::orc::GlowJIT> JIT, void *heap)
 
 CPUFunction::~CPUFunction() { alignedFree(heap_); }
 
-void CPUFunction::execute(llvm::ArrayRef<Placeholder *> placeholders,
-                          llvm::ArrayRef<Tensor *> tensors) {
+void CPUFunction::execute() {
   auto sym = JIT_->findSymbol("jitmain");
   assert(sym && "Unable to JIT the code!");
   using JitFuncType = void (*)(void);

lib/Backends/CPU/CPUFunction.h

@@ -38,8 +38,7 @@ class CPUFunction final : public CompiledFunction {
   ///@{
   ~CPUFunction() override;
 
-  void execute(llvm::ArrayRef<Placeholder *> placeholders,
-               llvm::ArrayRef<Tensor *> tensors) override;
+  void execute() override;
   ///@}
 };
 

lib/Backends/Interpreter/InterpreterFunction.cpp

@@ -31,6 +31,10 @@ InterpreterFunction::InterpreterFunction(std::unique_ptr<IRFunction> F)
     assert(!externalTensors_.count(w) && "The tensor is already registered");
     externalTensors_[w] = &v->getPayload();
   }
+
+  for (auto *W : F_->getWeights()) {
+    getOrCreateTensor(W);
+  }
 }
 
 InterpreterFunction::~InterpreterFunction() {
@@ -98,15 +102,7 @@ void InterpreterFunction::deleteTensor(const Value *v) {
   tensors_.erase(it);
 }
 
-void InterpreterFunction::execute(llvm::ArrayRef<Placeholder *> placeholders,
-                                  llvm::ArrayRef<Tensor *> tensors) {
-  // Register the placeholder values with the corresponding backing tensor.
-  // Note: the current implementation is not thread safe. See issue #1334.
-  for (int i = 0, e = placeholders.size(); i < e; i++) {
-    auto *w = F_->getWeightForNode(placeholders[i]);
-    externalTensors_[w] = tensors[i];
-  }
-
+void InterpreterFunction::execute() {
 // Do the forward pass.
 #define DEF_VALUE(CLASS, NAME)
 #define DEF_INSTR(CLASS, NAME)                                                 \

lib/Backends/Interpreter/InterpreterFunction.h

@@ -54,8 +54,7 @@ class InterpreterFunction final : public CompiledFunction {
   ///@{
   ~InterpreterFunction() override;
 
-  void execute(llvm::ArrayRef<Placeholder *> placeholders,
-               llvm::ArrayRef<Tensor *> tensors) override;
+  void execute() override;
   ///@}
 
 private:

lib/Backends/OpenCL/OpenCL.cpp

@@ -587,8 +587,7 @@ static void topK(Tensor &outW, Tensor &indW, Tensor &inW, size_t k) {
   }
 }
 
-void OpenCLFunction::execute(llvm::ArrayRef<Placeholder *> placeholders,
-                             llvm::ArrayRef<Tensor *> tensors) {
+void OpenCLFunction::execute() {
   auto copiedToDeviceBytes = copyMutableWeightsToDevice();
   (void)copiedToDeviceBytes;
   DEBUG_GLOW(llvm::dbgs() << "Copied " << copiedToDeviceBytes

lib/Backends/OpenCL/OpenCL.h

@@ -98,8 +98,7 @@ class OpenCLFunction final : public CompiledFunction {
   ///@{
   ~OpenCLFunction() override;
 
-  void execute(llvm::ArrayRef<Placeholder *> placeholders,
-               llvm::ArrayRef<Tensor *> tensors) override;
+  void execute() override;
   ///@}
 
 private:

lib/ExecutionEngine/ExecutionEngine.cpp

@@ -28,8 +28,6 @@
 
 using namespace glow;
 
-using llvm::cast;
-
 namespace {
 static llvm::cl::opt<std::string>
     dumpIRDAG("dump-ir-dag",
@@ -57,39 +55,24 @@ void ExecutionEngine::setBackend(Backend *backend) {
 
 ExecutionEngine::~ExecutionEngine() = default;
 
-void ExecutionEngine::run(llvm::ArrayRef<Storage *> vars,
+void ExecutionEngine::run(llvm::ArrayRef<Variable *> vars,
                           llvm::ArrayRef<Tensor *> inputs) {
   assert(function_ && "No function has been compiled");
   assert(inputs.size() == vars.size() &&
          "The number of inputs does not match the number of variables");
 
-  llvm::SmallVector<Placeholder *, 8> placeholders;
-  llvm::SmallVector<Tensor *, 8> tensors;
-
-  // Update the input variables, or register placeholders.
+  // Update the input variables.
   for (int i = 0, e = vars.size(); i < e; i++) {
-    // This is a variable. In here we override the content of the tensor with
-    // the requested tensor.
-    // Note: When we finish the migration to Placeholder this code needs to be
-    // removed because it's not threadsafe. See issue #1334.
-    if (Variable *V = llvm::dyn_cast<Variable>(vars[i])) {
-      assert(V->getVisibilityKind() == VisibilityKind::Public &&
-             "Trying to update a private variable");
-      loadValueFromTensor(V, inputs[i]);
-      continue;
-    }
-
-    // This is a placeholder that we need to make concrete during the execution
-    // of the program.
-    placeholders.push_back(cast<Placeholder>(vars[i]));
-    tensors.push_back(inputs[i]);
+    assert(vars[i]->getVisibilityKind() == VisibilityKind::Public &&
+           "Trying to update a private variable");
+    loadValueFromTensor(vars[i], inputs[i]);
   }
 
-  function_->execute(placeholders, tensors);
+  function_->execute();
 }
 
 void ExecutionEngine::runBatch(size_t iterations,
-                               llvm::ArrayRef<Storage *> vars,
+                               llvm::ArrayRef<Variable *> vars,
                                llvm::ArrayRef<Tensor *> inputs) {
   static size_t trainCounter = 0;
 
@@ -110,56 +93,16 @@ void ExecutionEngine::runBatch(size_t iterations,
   }
 }
 
-void ExecutionEngine::updateInputsAndRunNetwork(llvm::ArrayRef<Storage *> vars,
+void ExecutionEngine::updateInputsAndRunNetwork(llvm::ArrayRef<Variable *> vars,
                                                 llvm::ArrayRef<Tensor *> inputs,
                                                 size_t sampleIdx) {
-  llvm::SmallVector<Placeholder *, 8> placeholders;
-
-  // This container saves the tensor slices that were extracted from the inputs.
-  // The tensors are alive during the lifetime of this function. These tensors
-  // must not move, and we must pass them as ArrayRef, so we allocate them here
-  // manually.
-  llvm::SmallVector<Tensor, 8> tempTensorViews;
-  // We need to pass the tensors as array ref, so we reference the static
-  // storage above, that's frozen during the execution of the program.
-  llvm::SmallVector<Tensor *, 8> tensors;
-
   // Update the input variables.
   for (int i = 0, e = vars.size(); i < e; i++) {
-    // Note: When we finish the migration to Placeholder this code needs to be
-    // removed because it's not threadsafe. See issue #1334.
-    if (Variable *V = llvm::dyn_cast<Variable>(vars[i])) {
-      loadValueFromTensorSlice(V, inputs[i], sampleIdx);
-      continue;
-    }
-
-    // This is a placeholder that we need to make concrete during the execution
-    // of the program.
-    placeholders.push_back(cast<Placeholder>(vars[i]));
-
-    // Extract a tensor view from the input values. This has the same
-    // functionality as the logic in loadValueFromTensorSlice that copies the
-    // content of the tensor.
-    auto batchSize = inputs[i]->dims()[0];
-    auto startIdx = sampleIdx % batchSize;
-    auto phDims = placeholders[i]->dims();
-
-    // The start offset is all zeros except for the batch dimension.
-    std::vector<size_t> sliceOffset(inputs[i]->dims().size(), 0);
-    sliceOffset[0] = startIdx;
-
-    // Create the tensor view.
-    tempTensorViews.push_back(inputs[i]->getUnowned(phDims, sliceOffset));
-  }
-
-  // Save the pointer to the allocated tensor view, because the interface
-  // requires ArrayRef of tensors.
-  for (int i = 0, e = tempTensorViews.size(); i < e; i++) {
-    tensors.push_back(&tempTensorViews[i]);
+    loadValueFromTensorSlice(vars[i], inputs[i], sampleIdx);
   }
 
   // Run the network.
-  function_->execute(placeholders, tensors);
+  function_->execute();
 }
 
 void ExecutionEngine::loadValueFromTensorSlice(Variable *v, Tensor *input,

lib/Graph/Graph.cpp

@@ -2155,6 +2155,9 @@ void Function::verify() const {
     llvm_unreachable("Multiple nodes with the same name");
   }
 
+  const auto &vars = getParent()->getVars();
+  (void)vars;
+
   // Any node referenced by one of the graph nodes should be part of the Graph.
   for (const auto &N : nodes_) {
     for (size_t idx = 0, e = N.getNumInputs(); idx < e; ++idx) {

lib/Onnxifi/Base.cpp

@@ -68,7 +68,7 @@ onnxStatus Graph::initGraph(const void *onnxModel, size_t onnxModelSize,
 onnxStatus Graph::run() {
   // Copy tensors from the input addresses to the Glow tensors.
   llvm::SmallVector<Tensor *, 4> tensors;
-  llvm::SmallVector<Storage *, 4> vars;
+  llvm::SmallVector<Variable *, 4> vars;
   for (auto inputVar : inputVarToBuffer_) {
     auto *var = inputVar.first;
     auto *type = var->getType();

tests/unittests/BackendCorrectnessTest.cpp

@@ -304,7 +304,7 @@ TEST_P(CPUOnly, dataParallelStackingTest) {
   }
 
   MockCPUBackend backend;
-  backend.compile(std::move(M))->execute({}, {});
+  backend.compile(std::move(M))->execute();
   auto H = var->getHandle();
   EXPECT_EQ(H.at(0), 3);
   EXPECT_EQ(H.at(1), 4);

tests/unittests/BackendTest.cpp

@@ -143,7 +143,7 @@ TEST_P(BackendTest, debugPrint) {
 
   std::unique_ptr<Backend> backend(createBackend(GetParam()));
   auto function = backend->compile(std::move(IR));
-  function->execute({}, {});
+  function->execute();
 }
 
 /// This test checks that we can compile a function without depending on the
@@ -173,59 +173,6 @@ TEST_P(BackendTest, decoupleCodegenFromGraph) {
   EXPECT_NEAR(HX.at({2}), 9, 1E-5);
 }
 
-/// Check that we can pass information to the execution engine using Placeholder
-/// variables and read it back using Save nodes (in variables).
-TEST(Placeholder, simplePlaceholderValue) {
-  Tensor data{99.0, 35.0, 2.0, 3.0};
-
-  ExecutionEngine EE{BackendKind::Interpreter};
-  auto &mod = EE.getModule();
-
-  Function *F = mod.createFunction("main");
-  auto *input = mod.createPlaceholder(ElemKind::FloatTy, {4}, "input");
-  SaveNode *S = F->createSave("ret", input);
-
-  EE.compile(CompilationMode::Infer, F);
-
-  EE.run({input}, {&data});
-
-  auto &res = S->getVariable()->getPayload();
-  EXPECT_TRUE(res.isEqual(data));
-}
-
-/// Check that we can pass information to the execution engine using Placeholder
-/// variables and the runBatch API.
-TEST(Placeholder, runBatchTest) {
-  // The input contains two slices of 4 floats each.
-  Tensor data(ElemKind::FloatTy, {4, 4});
-  // Fill the array with the pattern: [0 1 2 3; 10, 11, 12, 13; 20 21 22 23 ...]
-  for (size_t i = 0; i < 4; i++) {
-    for (size_t j = 0; j < 4; j++) {
-      data.getHandle().at({i, j}) = i * 10 + j;
-    }
-  }
-
-  ExecutionEngine EE{BackendKind::Interpreter};
-  auto &mod = EE.getModule();
-
-  Function *F = mod.createFunction("main");
-  auto *input = mod.createPlaceholder(ElemKind::FloatTy, {1, 4}, "input");
-  SaveNode *S = F->createSave("ret", input);
-
-  EE.compile(CompilationMode::Infer, F);
-
-  // Run the batch for 2 iterations:
-  EE.runBatch(2, {input}, {&data});
-
-  Tensor expected{10, 11, 12, 13};
-  auto EH = expected.getHandle();
-  auto RH = S->getVariable()->getPayload().getHandle();
-
-  for (size_t i = 0; i < 4; i++) {
-    EXPECT_EQ(RH.raw(i), EH.raw(i));
-  }
-}
-
 INSTANTIATE_TEST_CASE_P(Interpreter, BackendTest,
                         ::testing::Values(BackendKind::Interpreter));
 

tests/unittests/BackendTestUtils.h

@@ -23,8 +23,7 @@ namespace glow {
 /// MockBackend used only for unit testing.
 class MockBackend : public Backend {
   class MockFunction : public CompiledFunction {
-    void execute(llvm::ArrayRef<Placeholder *> placeholders,
-                 llvm::ArrayRef<Tensor *> tensors) override {}
+    void execute() override {}
   };
   std::unique_ptr<CompiledFunction>
   compile(std::unique_ptr<IRFunction> IR) const override {

tests/unittests/partitionTest.cpp

@@ -35,7 +35,7 @@ class PartitionTest : public ::testing::Test {
 };
 
 /// Execute a graph of functions serially, which is the simplest approach.
-static void executeSerial(const FunctionDAG &G, llvm::ArrayRef<Storage *> vars,
+static void executeSerial(const FunctionDAG &G, llvm::ArrayRef<Variable *> vars,
                           llvm::ArrayRef<Tensor *> inputs) {
   for (auto *F : G.getFunctions()) {
     ExecutionEngine EE;

tools/loader/Loader.cpp

@@ -267,7 +267,7 @@ void Loader::compile() {
   }
 }
 
-void Loader::runInference(llvm::ArrayRef<Storage *> variables,
+void Loader::runInference(llvm::ArrayRef<Variable *> variables,
                           llvm::ArrayRef<Tensor *> tensors) {
   assert(!emittingBundle() &&
          "No inference is performed in the bundle generation mode.");

tools/loader/Loader.h

@@ -60,7 +60,7 @@ class Loader {
 
   /// Runs inference, unless emit bundle mode is enabled. If inference is run
   /// then it will \return true, else false.
-  void runInference(llvm::ArrayRef<Storage *> variables,
+  void runInference(llvm::ArrayRef<Variable *> variables,
                     llvm::ArrayRef<Tensor *> tensors);
 
   /// Create the Loader driver object, and parse/verify the command line