Change the input to NNAPI EP ModelBuilder from ModelProto to GraphViewer #4389
Conversation
This file contains 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
skottmckay
reviewed
Jul 2, 2020
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/model_builder.cc
Show resolved
Hide resolved
skottmckay
reviewed
Jul 2, 2020
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/model_builder.cc
Outdated
Show resolved
Hide resolved
skottmckay
reviewed
Jul 2, 2020
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/model_builder.cc
Show resolved
Hide resolved
skottmckay
reviewed
Jul 2, 2020
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/model_builder.cc
Show resolved
Hide resolved
skottmckay
reviewed
Jul 2, 2020
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/op_builder.cc
Outdated
Show resolved
Hide resolved
skottmckay
reviewed
Jul 2, 2020
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/op_builder.cc
Outdated
Show resolved
Hide resolved
skottmckay
reviewed
Jul 2, 2020
| bool BaseOpBuilder::HasExternalInitializer(ModelBuilder& model_builder, | ||
| const onnxruntime::Node& node) { | ||
| const auto& initializers(model_builder.GetOnnxGraph().GetAllInitializedTensors()); | ||
| for (const auto* node_arg : node.InputDefs()) { |
There was a problem hiding this comment.
Would probably be quicker to create a set of external intializer names once and just checking against that. #WontFix
There was a problem hiding this comment.
I guess this will be true if we have some reused initializers, will keep this as is for now since I do not really want to add another member variable to the ModelBuilder
In reply to: 448725831 [](ancestors = 448725831)
skottmckay
reviewed
Jul 2, 2020
… the caller, remove some redundant onnxruntime namespace
…er minor logging changes
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: Change the input to ModelBuilder from ModelProto to GraphViewer (No. 2 TODO in #4287)
Motivation and Context
In the previous iteration of the NNAPI EP, we use the ModelProto as the input to ModelBuilder, which has to be constructed from the Graph, which is not necessary and inefficient,
Changed to use the Graph_Viewer as the input of the ModelBuilder, which this change, it will be much easier to query the state of a node/tensor than using ModelProto
Not Included in This Change
This change is only to move from ModelProto to GraphViewer, while the functionalities remain the same as before, additional changes such as [Check the input/initializer of each individual operator while calling GetCapability()] (No. 8 TODO in #4287) will be addressed in future PRs