Enable calling into runtime from onnxifi #2300
Merged
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
nadavrot
reviewed
Jan 26, 2019
lib/Onnxifi/Base.cpp
Outdated
| @@ -22,17 +22,22 @@ | |||
|
|
|||
| namespace glow { | |||
| namespace onnxifi { | |||
| namespace { | |||
| const std::string inferenceFunctionName = "inference"; | |||
There was a problem hiding this comment.
Why is this std::string and not "const char*" ?
There was a problem hiding this comment.
No reason, I'll change this and the below std::string
There was a problem hiding this comment.
+1, those strings could cause SIOFs
rdzhabarov
reviewed
Jan 28, 2019
lib/Onnxifi/Base.cpp
Outdated
| @@ -22,17 +22,22 @@ | |||
|
|
|||
| namespace glow { | |||
| namespace onnxifi { | |||
| namespace { | |||
| const std::string inferenceFunctionName = "inference"; | |||
There was a problem hiding this comment.
+1, those strings could cause SIOFs
| bool use_onnx) | ||
| : id_(id), use_onnx_(use_onnx), concurrency_(concurrency), | ||
| executionEngine_(kind) {} | ||
| bool useOnnx, bool useHostManager) |
There was a problem hiding this comment.
update doxygen with the useHostManager.
gcatron
approved these changes
Jan 28, 2019
rdzhabarov
reviewed
Jan 28, 2019
lib/Onnxifi/Base.cpp
Outdated
| @@ -146,26 +156,42 @@ void Graph::run( | |||
| phs.push_back(var); | |||
| } | |||
|
|
|||
| auto ctx = std::make_unique<Context>(); | |||
There was a problem hiding this comment.
make this llvm::make_unique.
@bertmaher recently fixed bunch of places to make glow g++-4.8.5 compatible.
jackm321
force-pushed
the
onnxifi_enable_runtime_usage
branch
from
a911d58 to
61050d5
Compare
jackm321
force-pushed
the
onnxifi_enable_runtime_usage
branch
from
61050d5 to
0f5608b
Compare
|
Landing this now so we can keep moving with refactoring the onnxifi interface to get ready for full HostManager integration once it's ready. Will add integration tests once HostManager is ready. |
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.
Description:
Enable calling into the Glow runtime from ONNXIFI through the HostManager.
This diff enables running onnxifi inference through HostManager once HostManager is implemented.
This diff is intended to get end-to-end support for the runtime working but there are many things that we cannot yet do such as multiple runs with the same Graph, running multiple onnxifi Graphs on the same onnxifi Backend, etc. To enable these things, some more larger refactoring steps will be needed in followups including 1. Moving all resources necessary for running graphs into BackendId, 2. Abstracting BackendId to enable separate implementations for HostManager and ExecutionEngine backed BackendIds (module is still always owned by ExecutionEngine no matter what), 3 move all graph state into the graph itself to enable running multiple times with different contexts, 4 move components necessary for compatibility check into runtime and separate the ExecutionEngine from the HostManager implementations.
Depends on #2284.
Testing:
ninja test, CIDocumentation:
doxygen