From 4e2a03d5faa60b1aa4049567147e93187aa38a92 Mon Sep 17 00:00:00 2001 From: Adrian Lizarraga Date: Thu, 12 Oct 2023 23:37:01 -0700 Subject: [PATCH] [QNN EP] Fix topological node unit traversal during validation (#17913) ### Description We need to ensure that tensors are first created and validated by their producers. If we don't, then builders that need to modify their outputs may not be able to do so if consumers are processed first (due to caching of tensors). For example, the Tanh builder may need to override its output quant param for 16-bit QDQ. I've encountered a scenario (while working on a partner model) where the override was not being correctly applied due to the graph traversal order. I tried to fix this bug in a previous [PR](https://github.com/microsoft/onnxruntime/pull/17877#discussion_r1353676802), but my fix was incorrect. --- .../qnn/builder/opbuilder/simple_op_builder.cc | 6 +++++- .../core/providers/qnn/qnn_execution_provider.cc | 13 ++++++++++++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/onnxruntime/core/providers/qnn/builder/opbuilder/simple_op_builder.cc b/onnxruntime/core/providers/qnn/builder/opbuilder/simple_op_builder.cc index acdcfdc66b..4022307b93 100644 --- a/onnxruntime/core/providers/qnn/builder/opbuilder/simple_op_builder.cc +++ b/onnxruntime/core/providers/qnn/builder/opbuilder/simple_op_builder.cc @@ -371,10 +371,14 @@ Status SimpleOpBuilder::ProcessSigmoidOrTanhOutput(QnnModelWrapper& qnn_model_wr const float scale = output_info.quant_param.scaleOffsetEncoding.scale; LOGS(logger, VERBOSE) << "QNN requires that 16-bit quantized " << op_type << " operators use offset/scale values " - << "of <" << offset << ", " << scale << ">. QNN EP will override the original values."; + << "of <" << offset << ", " << scale << ">. QNN EP will override the original values for output " + << output_name; } } + ORT_RETURN_IF(qnn_model_wrapper.IsQnnTensorWrapperExist(output_name), + "QNN EP is unable to override output quantization parameters for ", op_type.c_str(), + " operator. Node name: ", node_unit.Name().c_str(), ", output name: ", output_name.c_str()); Qnn_TensorType_t tensor_type = qnn_model_wrapper.IsGraphOutput(output_name) ? QNN_TENSOR_TYPE_APP_READ : QNN_TENSOR_TYPE_NATIVE; QnnTensorWrapper output_tensorwrapper(output_name, tensor_type, output_info.qnn_data_type, output_info.quant_param, diff --git a/onnxruntime/core/providers/qnn/qnn_execution_provider.cc b/onnxruntime/core/providers/qnn/qnn_execution_provider.cc index d497bc1c06..b456be9241 100644 --- a/onnxruntime/core/providers/qnn/qnn_execution_provider.cc +++ b/onnxruntime/core/providers/qnn/qnn_execution_provider.cc @@ -242,7 +242,15 @@ QNNExecutionProvider::GetSupportedNodes(const GraphViewer& graph_viewer, for (size_t i = 0; i < node_indices.size(); i++) { gsl::not_null node(graph_viewer.GetNode(node_indices[i])); + // Get the node_unit associated with the node. Note that the node may not be the node_unit's target node. const NodeUnit* node_unit = node_unit_map.at(node); + + // Visiting 'nodes' in topological order does not guarantee that 'node_units' are + // also visited in topological order. Skip this node if it is not the node_unit's target node + // to ensure 'node_units' are visited in topological order. + if (node != &node_unit->GetNode()) { + continue; + } const bool supported = IsNodeSupported(qnn_model_wrapper, *node_unit, node_unit_supported_result, @@ -256,7 +264,10 @@ QNNExecutionProvider::GetSupportedNodes(const GraphViewer& graph_viewer, << "] name: [" << node_unit->Name() << "]"; if (supported) { - supported_nodes.insert(node); + // If the node_unit is supported, add all of its nodes to the supported list. + for (const auto* node_in_group : node_unit->GetAllNodesInGroup()) { + supported_nodes.insert(node_in_group); + } } }