Enable transpose optimizer in minimal extended build #10349

edgchen1 · 2022-01-21T02:45:17Z

Description
Enable transpose optimizer and infrastructure it depends on in a minimal extended build.

Motivation and Context
Enable the transpose optimizer to run as part of runtime optimization of ORT format models.

…optimizer_minimal_build

…tended builds.

…RUNTIME_OPTIMIZATIONS_IN_MINIMAL_BUILD.

…optimizer_minimal_build

guoyu-wang · 2022-01-21T22:44:53Z

onnxruntime/core/session/inference_session.cc

+    if (node.ContainsSubgraph()) {
+      const auto subgraphs = node.GetSubgraphs();
+      for (const auto& subgraph : subgraphs) {
+        const auto status = VerifyEachNodeIsAssignedToAnEpImpl(*subgraph, is_verbose, node_placements);


nit, ORT_RETURN_IF_ERROR #Resolved

guoyu-wang · 2022-01-21T22:45:08Z

onnxruntime/core/graph/graph_utils.h

+
+/** Returns the attribute of a Node with a given name. */
+const ONNX_NAMESPACE::AttributeProto* GetNodeAttribute(const Node& node, const std::string& attr_name);
+


nit, can be moved below with AddInitializer to reduce number of #ifdef? #Resolved

this one is unfortunately used by the template GetRepeatedNodeAttributeValues(), which is above the other ifdef

Can we move the other 2 functions up here in the same #ifdef?

skottmckay · 2022-01-25T06:20:42Z

cmake/external/onnx_minimal.cmake

+  "${ONNX_SOURCE_ROOT}/onnx/defs/data_type_utils.h"
+  "${ONNX_SOURCE_ROOT}/onnx/defs/data_type_utils.cc"
+  "${ONNX_SOURCE_ROOT}/onnx/defs/shape_inference.h"
+  "${ONNX_SOURCE_ROOT}/onnx/defs/shape_inference.cc"
 )


Why do we need the shape inference code?

I would have expected this causes a large hit to binary size but maybe most of it can be thrown away. #Closed

graph.cc:MergeShapeInfo():

onnxruntime/onnxruntime/core/graph/graph.cc

Line 66 in 481b96d

static Status MergeShapeInfo(const std::string& output_name,

NodeArg::UpdateTypeAndShape()
api_impl.cc:ApiGraph::CopyValueInfo()

onnxruntime/onnxruntime/core/optimizer/transpose_optimizer/api_impl.cc

Line 690 in 481b96d

ORT_THROW_IF_ERROR(dst_arg.UpdateTypeAndShape(*src_arg, /*strict*/ false, /*override_types*/ false, logger_));

the reason is ApiGraph::CopyValueInfo()

removed the dependency on shape inference code

skottmckay · 2022-01-25T06:23:52Z

Does this need to be enabled? I think in training they have scenarios when the type gets overridden, but for our usages I wouldn't expect that to ever happen.

In reply to: 1020851523

In reply to: 1020851523 #Closed

Refers to: include/onnxruntime/core/graph/node_arg.h:86 in 16c7768. [](commit_id = 16c7768, deletion_comment = False)

skottmckay · 2022-01-25T06:38:11Z

Actually taking a step up, do we need to use UpdateTypeAndShape?

I could only see a call to that in ApiGraph::CopyValueInfo, and given that's a straight copy we shouldn't need any of the merge/override logic. Not sure what isn't populated by the call to GetOrCreateNodeArg with the source NodeArg - if that include type and shape we don't need a call to UpdateTypeAndShape at all.

In reply to: 1020851523

Refers to: include/onnxruntime/core/graph/node_arg.h:86 in 16c7768. [](commit_id = 16c7768, deletion_comment = False)

skottmckay · 2022-01-25T06:44:51Z

onnxruntime/core/graph/graph.cc

+
+  for (const auto& node : Nodes()) {
+    node.ForEachDef([&](const NodeArg& node_arg, bool is_input) {
+      if (is_input) {


nice! way simpler than the old code :) #Closed

…optimizer_minimal_build

edgchen1 · 2022-01-27T20:17:25Z

yeah, modifying ApiGraph::CopyValueInfo might be better

In reply to: 1020858991

Refers to: include/onnxruntime/core/graph/node_arg.h:86 in 16c7768. [](commit_id = 16c7768, deletion_comment = False)

…optimizer_minimal_build

TomWildenhain-Microsoft · 2022-01-29T04:10:18Z

onnxruntime/core/optimizer/transpose_optimizer/api_impl.cc

+  NodeArg& dst_arg = graph_.GetOrCreateNodeArg(std::string(dst_name), nullptr);
+
+  if (auto* dst_type = dst_arg.TypeAsProto(); dst_type != nullptr) {
+    int32_t src_data_element_type;


The caller guarantees this so you don't have to do the check if you don't want to.

That's good to know. Will leave the check to make debugging easier.

TomWildenhain-Microsoft · 2022-01-29T04:11:24Z

onnxruntime/core/optimizer/transpose_optimizer/api.h

-  /// in the ValueInfo class (like symbolic shape information). Destination dtype must be compatible with source dtype
-  /// if known.
+  /// in the ValueInfo class (like symbolic shape information). If already set, the destination dtype should be equal
+  /// to the source dtype.


This is correct, and if the caller fails to do this, the behavior is undefined.

skottmckay · 2022-01-31T04:58:08Z

onnxruntime/core/optimizer/transpose_optimizer/api_impl.cc

+  NodeArg& dst_arg = graph_.GetOrCreateNodeArg(std::string(dst_name), nullptr);
+
+  if (auto* dst_type = dst_arg.TypeAsProto(); dst_type != nullptr) {
+    int32_t src_data_element_type;


does the dst_type init have to be in the 'if'? just wondering what the benefit of that is given it slightly complicates reading the code.

the benefit in my opinion is that the scope of dst_type can be more limited

skottmckay

🕐

skottmckay

edgchen1 added 11 commits December 17, 2021 10:07

Save initial changes.

b38ecdd

Include onnx shape_inference.cc in minimal build.

e230b4a

Pull out logic for populating NodeArg to producer/consumer lookups.

0f7f758

Merge remote-tracking branch 'origin/master' into edgchen1/transpose_…

9c9bc54

…optimizer_minimal_build

Merge remote-tracking branch 'origin/master' into edgchen1/transpose_…

6909184

…optimizer_minimal_build

Call PopulateNodeArgToProducerConsumerLookupsFromNodes() from full/ex…

fad592f

…tended builds.

Rename ENABLE_RUNTIME_OPTIMIZATION_REPLAY_IN_MINIMAL_BUILD to ENABLE_…

64c9140

…RUNTIME_OPTIMIZATIONS_IN_MINIMAL_BUILD.

Update comment.

8954e99

Change ifdefs to enable stuff only if runtime optimizations are enabled.

f35b72d

Fix and clean up.

2611ace

Merge remote-tracking branch 'origin/master' into edgchen1/transpose_…

e2a0893

…optimizer_minimal_build

edgchen1 requested review from skottmckay, askhade, YUNQIUGUO and guoyu-wang January 21, 2022 02:45

edgchen1 added 2 commits January 20, 2022 20:18

Fix build.

ea6072f

Move GetNodeAttribute() to runtime opt ifdef.

90e1de5

guoyu-wang reviewed Jan 21, 2022

View reviewed changes

Address PR comments.

16c7768

edgchen1 changed the title ~~[WIP] Enable transpose optimizer in minimal extended build~~ Enable transpose optimizer in minimal extended build Jan 24, 2022

edgchen1 marked this pull request as ready for review January 24, 2022 17:42

skottmckay reviewed Jan 25, 2022

View reviewed changes

Merge remote-tracking branch 'origin/master' into edgchen1/transpose_…

694df7c

…optimizer_minimal_build

edgchen1 added 2 commits January 28, 2022 13:35

Remove dependency on ONNX shape inference code.

de84210

Merge remote-tracking branch 'origin/master' into edgchen1/transpose_…

594854e

…optimizer_minimal_build

edgchen1 added 4 commits January 28, 2022 14:14

Revert emsdk change.

8e8a57d

Fix build issues.

9b596a9

Fix.

afd97bb

Remove unused include.

7a1d145

edgchen1 requested a review from TomWildenhain-Microsoft January 28, 2022 23:28

Remove unused variable.

284c311

TomWildenhain-Microsoft reviewed Jan 29, 2022

View reviewed changes

skottmckay reviewed Jan 31, 2022

View reviewed changes

skottmckay requested changes Jan 31, 2022

View reviewed changes

skottmckay approved these changes Jan 31, 2022

View reviewed changes

edgchen1 merged commit c43c169 into master Jan 31, 2022

edgchen1 deleted the edgchen1/transpose_optimizer_minimal_build branch January 31, 2022 17:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable transpose optimizer in minimal extended build #10349

Enable transpose optimizer in minimal extended build #10349

edgchen1 commented Jan 21, 2022

guoyu-wang Jan 21, 2022 •

edited by edgchen1

Loading

guoyu-wang Jan 21, 2022 •

edited by edgchen1

Loading

edgchen1 Jan 21, 2022

guoyu-wang Jan 21, 2022

skottmckay Jan 25, 2022 •

edited

Loading

edgchen1 Jan 27, 2022

edgchen1 Jan 28, 2022

skottmckay commented Jan 25, 2022 •

edited

Loading

skottmckay commented Jan 25, 2022

skottmckay Jan 25, 2022 •

edited

Loading

edgchen1 commented Jan 27, 2022

TomWildenhain-Microsoft Jan 29, 2022

edgchen1 Jan 31, 2022

TomWildenhain-Microsoft Jan 29, 2022

skottmckay Jan 31, 2022

edgchen1 Jan 31, 2022

skottmckay left a comment

skottmckay left a comment


		/** Returns the attribute of a Node with a given name. */
		const ONNX_NAMESPACE::AttributeProto* GetNodeAttribute(const Node& node, const std::string& attr_name);

Enable transpose optimizer in minimal extended build #10349

Enable transpose optimizer in minimal extended build #10349

Conversation

edgchen1 commented Jan 21, 2022

guoyu-wang Jan 21, 2022 • edited by edgchen1 Loading

Choose a reason for hiding this comment

guoyu-wang Jan 21, 2022 • edited by edgchen1 Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skottmckay Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skottmckay commented Jan 25, 2022 • edited Loading

skottmckay commented Jan 25, 2022

skottmckay Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

edgchen1 commented Jan 27, 2022

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

skottmckay left a comment

Choose a reason for hiding this comment

skottmckay left a comment

Choose a reason for hiding this comment

guoyu-wang Jan 21, 2022 •

edited by edgchen1

Loading

guoyu-wang Jan 21, 2022 •

edited by edgchen1

Loading

skottmckay Jan 25, 2022 •

edited

Loading

skottmckay commented Jan 25, 2022 •

edited

Loading

skottmckay Jan 25, 2022 •

edited

Loading