Allow Compilation without Collecting Constants #2246

gcatron · 2019-01-08T23:28:39Z

Description: For the new runtime we need to be able to compile a function but collect the constants slightly later in the initialization process. This PR introduces a compileWithoutConstants method to the backend and updates existing backends to support it.
Testing: passes all unit tests.
Documentation: N/A
Fixes: #2254

nadavrot · 2019-01-09T05:21:05Z

@gcatron Please make sure to update the docs (see #2226) and communicate the plan. We need to make sure that people who are developing Glow backends are not surprised by these changes. Please make sure to update the PR with the overall plan and send an email with link to the PR.

#2226

nickgg · 2019-01-09T17:28:47Z

There's new behaviour here (compiling without collecting) that I don't think we're testing anywhere. Can we write a test to cover this?

bertmaher

Sorry, but I'm having trouble seeing the desired end-state here. Can you explain in a bit more detail why we need to split out constant collection?

include/glow/Backends/Backend.h

bertmaher

Sorry I'm pretty repetitive on the "blank lines separating definitions" theme, but that is a major bugaboo for me ;-).

bertmaher · 2019-01-09T18:47:42Z

lib/Backends/CPU/CPUBackend.h

@@ -45,7 +45,11 @@ class CPUBackend : public BackendUsingGlowIR {
  std::unique_ptr<CompiledFunction>
  compileIR(std::unique_ptr<IRFunction> IR) const override;

+  std::unique_ptr<CompiledFunction>
+  compileIRWithoutConstants(IRFunction *IR) const;


vertical space b/w top level declarations.

bertmaher · 2019-01-09T18:47:52Z

lib/Backends/CPU/CPUBackend.h

  std::unique_ptr<CompiledFunction> compile(Function *F) const override;
+  std::unique_ptr<CompiledFunction>


vertical space b/w top level declarations.

bertmaher · 2019-01-09T18:53:31Z

lib/Backends/CPU/CPUFunction.h

  /// Base address for Activations memory block.
  uint8_t *baseActivationsAddress_{};
  /// Base address for Mutable weights memory block, Inputs and Outputs.
  uint8_t *baseMutableWeightVarsAddress_{};

 public:
+  /// runtimeBundle contains symbol offsets and allocation sizes.
+  runtime::RuntimeBundle runtimeBundle_;


I recommend keeping runtimeBundle private; having public data members has a way of bloating the API. It seems like you should be able to just add a collectConstants method to CPUFunction. (Or maybe even to the CompiledFunction interface, since it's used by every backend.)

Or maybe even to the CompiledFunction interface, since it's used by every backend.

this way we'd also avoid ugly static_casts on pointers to specific CompiledFunctions.

I think adding it as a method to each compiledFunction makes sense, I think eventually this will go away entirely, the new DeviceManager will have the collection logic. Unfortunately even if I added it to CompiledFunction OpenCL and Interpreter will still require a static_cast to get the IR.

So how about I keep bundle private and add a new method to each CPUFunction, OpenCLFunction, and InterpreterFunction but not to compiledFunction, since ultimately we'd want to move this to the DeviceManager anyway?

bertmaher · 2019-01-09T18:53:48Z

lib/Backends/Interpreter/Interpreter.cpp

+Interpreter::compileWithoutConstants(Function *F) const {
+  auto IR = generateAndOptimizeIR(F, shouldShareBuffers());
+  return compileIRWithoutConstants(std::move(IR));
+}


Blank line between definitions.

bertmaher · 2019-01-09T18:54:07Z

lib/Backends/Interpreter/Interpreter.h


  std::unique_ptr<CompiledFunction> compile(Function *F) const override;
+  std::unique_ptr<CompiledFunction>


More blank lines plz :)

bertmaher · 2019-01-09T18:56:21Z

lib/Backends/Interpreter/InterpreterFunction.h

@@ -74,6 +74,8 @@ class InterpreterFunction final : public CompiledFunction {
  void tearDownRuns() override;

  void execute() override;
+  /// Get reference to IR function.
+  IRFunction *getIR() { return F_.get(); }


As above, I think adding a collectConstants would be better than adding getIR just to pass it back to runtime bundle.

The challenge here is the CPUFunction, it doesn't keep the IR around so it would still need it passed in.

That is a good point. Which makes me wonder, how could we even use collectConstants with the CPU backend? Since compileIR takes ownership of the IR and then destroys it, how can we later pass it to collect? (Right now we only collect inside the compile function, but that's going to change soon... right?

Yes, the device manager will have a reference to the module so that is where it can collect constants from.

Looking at collectConstants, does it even need to take an IRFunction? It looks like it pulls all the constants from the Module via F->getGraph()->getParent()->getConstants(). It then uses getWeightForNode to turn those back into WeightVars but I don't see a reason for that, since all we do is call getSizeInBytes, which we can get without getting the WeightVar. Am I missing something?

rdzhabarov · 2019-01-10T00:50:41Z

lib/Backends/BackendUtils.cpp

@@ -143,6 +143,5 @@ glow::generateRuntimeBundle(const IRFunction &F,

  runtime::RuntimeBundle info(symbolTable, constantMaxSize, placeholderMaxSize,


nit: just return this right away

rdzhabarov · 2019-01-10T00:58:12Z

lib/Backends/CPU/CPUFunction.h

  /// Base address for Activations memory block.
  uint8_t *baseActivationsAddress_{};
  /// Base address for Mutable weights memory block, Inputs and Outputs.
  uint8_t *baseMutableWeightVarsAddress_{};

 public:
+  /// runtimeBundle contains symbol offsets and allocation sizes.
+  runtime::RuntimeBundle runtimeBundle_;


Or maybe even to the CompiledFunction interface, since it's used by every backend.

this way we'd also avoid ugly static_casts on pointers to specific CompiledFunctions.

rdzhabarov · 2019-01-10T01:00:30Z

include/glow/Backends/Backend.h

@@ -42,6 +42,9 @@ class Backend {

  /// Generate code for input function \param F.
  virtual std::unique_ptr<CompiledFunction> compile(Function *F) const = 0;
+  /// Generate code for input function \param F but do not collect constants.
+  virtual std::unique_ptr<CompiledFunction>
+  compileWithoutConstants(Function *F) const = 0;


where is it invoked?

Currently nowhere but it will be called from the Provisioner.

bertmaher · 2019-01-11T17:45:09Z

lib/Backends/CPU/CPUBackend.cpp

+std::unique_ptr<CompiledFunction>
+CPUBackend::compileWithoutConstants(Function *F) const {
+  auto IR = generateAndOptimizeIR(F, shouldShareBuffers());
+  return compileIRWithoutConstants(IR.get());


Did we come to a conclusion about how collectConstants is going to work for the CPU backend yesterday? Since compileWithoutConstants throws away the IR, we don't have anything to pass to collectConstants...

I think we decided to make expose collect constants as a method on each compiled function, and keep the bundle_ private. We call collectConstants in CompileIR where we still have the IR.

bertmaher

Thanks, lgtm!

… Interpreter Backends. This will allow for compilation of the function and collection of the constants to be seperated which is needed for the new Runtime design Added unittest for compileWithoutConstants

facebook-github-bot added the CLA Signed label Jan 8, 2019

bertmaher requested changes Jan 9, 2019

View reviewed changes

bertmaher reviewed Jan 9, 2019

View reviewed changes

include/glow/Backends/Backend.h Show resolved Hide resolved

bertmaher reviewed Jan 9, 2019

View reviewed changes

rdzhabarov reviewed Jan 10, 2019

View reviewed changes

bertmaher reviewed Jan 11, 2019

View reviewed changes

bertmaher approved these changes Jan 15, 2019

View reviewed changes

gcatron force-pushed the allow_compile_without_constants branch from 31823b0 to 983b943 Compare January 16, 2019 00:02

Added compileWithoutConstants to backend and updated OpenCL, CPU, and…

488803e

… Interpreter Backends. This will allow for compilation of the function and collection of the constants to be seperated which is needed for the new Runtime design Added unittest for compileWithoutConstants

gcatron force-pushed the allow_compile_without_constants branch from c06d3ba to 488803e Compare January 16, 2019 19:14

gcatron merged commit 3aa4961 into pytorch:master Jan 16, 2019

gcatron deleted the allow_compile_without_constants branch January 16, 2019 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Compilation without Collecting Constants #2246

Allow Compilation without Collecting Constants #2246

gcatron commented Jan 8, 2019 •

edited

Loading

nadavrot commented Jan 9, 2019

nickgg commented Jan 9, 2019

bertmaher left a comment

bertmaher left a comment

bertmaher Jan 9, 2019

bertmaher Jan 9, 2019

bertmaher Jan 9, 2019

rdzhabarov Jan 10, 2019

gcatron Jan 10, 2019

bertmaher Jan 9, 2019

bertmaher Jan 9, 2019

bertmaher Jan 9, 2019

gcatron Jan 10, 2019

bertmaher Jan 10, 2019

gcatron Jan 11, 2019

bertmaher Jan 11, 2019

rdzhabarov Jan 10, 2019

rdzhabarov Jan 10, 2019

rdzhabarov Jan 10, 2019

gcatron Jan 10, 2019

bertmaher Jan 11, 2019

gcatron Jan 11, 2019

bertmaher left a comment

		std::unique_ptr<CompiledFunction> compile(Function *F) const override;
		std::unique_ptr<CompiledFunction>

		@@ -143,6 +143,5 @@ glow::generateRuntimeBundle(const IRFunction &F,

		runtime::RuntimeBundle info(symbolTable, constantMaxSize, placeholderMaxSize,

Allow Compilation without Collecting Constants #2246

Allow Compilation without Collecting Constants #2246

Conversation

gcatron commented Jan 8, 2019 • edited Loading

nadavrot commented Jan 9, 2019

nickgg commented Jan 9, 2019

bertmaher left a comment

Choose a reason for hiding this comment

bertmaher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bertmaher left a comment

Choose a reason for hiding this comment

gcatron commented Jan 8, 2019 •

edited

Loading