Addressed PR feedback

onnxruntime/core/optimizer/pad_fusion.cc

@@ -32,14 +32,15 @@ bool PadFusion::SatisfyCondition(const Graph& graph, const Node& node, const log
     return false;
   }
 
-  // constant_value should be an initializer because we have to verify the constant_value should be zero.
-  // It is because Conv and MaxPool allow only 0 as padding value.
-  if (node.InputDefs().size() > 2 && !graph_utils::NodeArgIsConstant(graph, *node.InputDefs()[2])) {
-    return false;
-  }
+  // Since opset 11, <pads> and <constant_value> moved to inputs.
+  // Both of these should be initializer because we have to verify the values.
+  if (node.SinceVersion() >= 11) {
+    if (!graph_utils::NodeArgIsConstant(graph, *node.InputDefs()[1]) ||
+       (node.InputDefs().size() > 2 && !graph_utils::NodeArgIsConstant(graph, *node.InputDefs()[2]))) {
+      return false;
+    }
 
-  // Since opset 11, pad constant value becomes part of input instead of attribute.
-  if (node.InputDefs().size() > 2) {
+    // constant_value should be zero because Conv and MaxPool allow only 0 as padding value.
     const auto* pad_constant_value_proto = graph_utils::GetConstantInitializer(graph, node.InputDefs()[2]->Name());
     Initializer pad_constant_value{*pad_constant_value_proto, graph.ModelPath()};
     if (std::any_of(pad_constant_value.DataAsByteSpan().begin(), pad_constant_value.DataAsByteSpan().end(), [](const uint8_t byte) { return byte != 0; })) {
@@ -57,6 +58,22 @@ bool PadFusion::SatisfyCondition(const Graph& graph, const Node& node, const log
       !graph_utils::IsSupportedOptypeVersionAndDomain(child_node, "MaxPool", {1, 8, 10, 11, 12})) {
     return false;
   }
+
+  // Don't fuse if MaxPool has optional output indices tensor because output indices tensor 
+  // does not incorporate pad values. Basically if we allow the fusion, then dimension values 
+  // of input tensor < dimension values of input tensor without fusion. 
+  // This will cause the range of values for output indices tensor to be less than what it 
+  // should have been.
+
+  if (child_node.OutputDefs().size() > 1) {
+    return false;
+  }
+
+  // conv or maxpool node must use explicit padding to perform this fusion.
+  if (child_node.GetAttributes().find("auto_pad") != child_node.GetAttributes().end() &&
+      child_node.GetAttributes().at("auto_pad").s() != "NOTSET") {
+    return false;
+  }
   return true;
 }
 
@@ -74,6 +91,9 @@ Status PadFusion::Apply(Graph& graph, Node& pad_node, RewriteRuleEffect& rule_ef
     pads_values.assign(pad_node.GetAttributes().at("pads").ints().begin(), pad_node.GetAttributes().at("pads").ints().end());
   }
 
+  uint32_t input_rank = static_cast<uint32_t>(pad_node.InputDefs()[0]->Shape()->dim_size());
+  assert(static_cast<uint32_t>(pads_values.size()) == (2 * input_rank));
+
   uint32_t pads_size = static_cast<uint32_t>(pads_values.size());
   // check if padding is applied only on feature dims
   if (pads_values[0] != 0 || pads_values[1] != 0 || pads_values[pads_size / 2] != 0 ||
@@ -92,7 +112,9 @@ Status PadFusion::Apply(Graph& graph, Node& pad_node, RewriteRuleEffect& rule_ef
 
   for (uint32_t pads_index = 2, child_index = 0; pads_index < pads_size / 2; pads_index++, child_index++) {
     child_pads->Set(child_index, child_pads->Get(child_index) + pads_values[pads_index]);
-    child_pads->Set(child_index + (child_pads_size / 2), child_pads->Get(child_index + (child_pads_size / 2)) + pads_values[pads_index + (pads_size / 2)]);
+    uint32_t mirrored_child_index = child_index + (child_pads_size / 2);
+    uint32_t mirrored_pad_index = pads_index + (pads_size / 2);
+    child_pads->Set(mirrored_child_index, child_pads->Get(mirrored_child_index) + pads_values[mirrored_pad_index]);
   }
 
   graph_utils::RemoveNodeOutputEdges(graph, pad_node);