Address PR comments

onnxruntime/core/providers/coreml/builders/coreml_spec.h

@@ -19,11 +19,11 @@
 #endif
 #elif defined(_MSC_VER)
 #pragma warning(push)
-#pragma warning(disable : 4244)
+#pragma warning(disable : 4244)  // conversion from long to int
 #endif
 
 // Model.pb.h is generated in the build output directory from the CoreML protobuf files in
-// onnxruntime/core/providers/coreml/coremltools/mlmodel/format
+// <build output directory>/_deps/coremltools-src/mlmodel/format
 #include "coreml_proto/Model.pb.h"
 
 #if defined(__GNUC__)

onnxruntime/core/providers/coreml/builders/impl/builder_utils.h

@@ -10,8 +10,8 @@
 #include "core/common/gsl.h"
 #include "core/common/status.h"
 #include "core/graph/basic_types.h"
-#include "core/providers/coreml/builders/coreml_spec.h"
 #include "core/providers/common.h"
+#include "core/providers/coreml/builders/coreml_spec.h"
 #include "core/providers/shared/utils/utils.h"
 
 namespace onnxruntime {

onnxruntime/core/providers/coreml/builders/impl/clip_op_builder.cc

@@ -92,16 +92,16 @@ Status ClipOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder,
         Operation& clip_op = *op;
         AddOperationInput(clip_op, "x", input_name);
 
-        // if min and max were attributes we need to add initializers. otherwise we use the existing ones
+        // if min and max were attributes we need to add initializers. otherwise we use the existing inputs
         const bool min_max_attribs = node.SinceVersion() < 11;
-        const std::string& min_name = min_max_attribs ? model_builder.AddScalarConstant(clip_op.type(), "min", min)
-                                                      : node.InputDefs()[1]->Name();
+        std::string_view min_name = min_max_attribs ? model_builder.AddScalarConstant(clip_op.type(), "min", min)
+                                                    : node.InputDefs()[1]->Name();
 
         AddOperationInput(clip_op, "alpha", min_name);
 
         if (has_max) {
-          const std::string& max_name = min_max_attribs ? model_builder.AddScalarConstant(clip_op.type(), "max", max)
-                                                        : node.InputDefs()[2]->Name();
+          std::string_view max_name = min_max_attribs ? model_builder.AddScalarConstant(clip_op.type(), "max", max)
+                                                      : node.InputDefs()[2]->Name();
           AddOperationInput(clip_op, "beta", max_name);
         }
       }

onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc

@@ -145,12 +145,12 @@ Status GemmOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const N
         AddOperationInput(*gemm_op, "weight", b.Name());
       } else {
         // transpose from {K, N} to {N, K}
-        std::vector<float> weight_t;
-        std::vector<int64_t> weight_t_shape = {N, K};
-        ORT_RETURN_IF_ERROR(GetTensorFloatDataTransposed(*b_initializer, weight_t));
+        std::vector<float> weight_nk;
+        std::vector<int64_t> weight_nk_shape = {N, K};
+        ORT_RETURN_IF_ERROR(GetTensorFloatDataTransposed(*b_initializer, weight_nk));
 
         AddOperationInput(*gemm_op, "weight",
-                          model_builder.AddConstant(gemm_op->type(), b.Name() + "_t", weight_t, weight_t_shape));
+                          model_builder.AddConstant(gemm_op->type(), b.Name() + "_t", weight_nk, weight_nk_shape));
       }
 
       if (input_defs.size() == 3) {

onnxruntime/core/providers/coreml/builders/impl/pool_op_builder.cc

@@ -66,9 +66,9 @@ Status PoolOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder,
       AddOperationInput(*op, "keep_dims", model_builder.AddScalarConstant(op->type(), "keep_dims", true));
     } else {
       NodeAttrHelper helper(node);
-      const int num_spatial_dims = 2;  // we only support 4D. -2 for N and C dims.
+      constexpr int num_spatial_dims = 2;  // we only support 4D. -2 for N and C dims.
 
-      AddPadTypeAndPads(*op, model_builder, op->type(), helper, 2);
+      AddPadTypeAndPads(*op, model_builder, op->type(), helper, num_spatial_dims);
 
       const auto kernel_shape = helper.GetInt64s("kernel_shape");  // required
       AddOperationInput(*op, "kernel_sizes", model_builder.AddConstant(op->type(), "kernel_sizes", *kernel_shape));
@@ -82,7 +82,7 @@ Status PoolOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder,
       AddOperationInput(*op, "ceil_mode", model_builder.AddScalarConstant(op->type(), "ceil_mode", ceil_mode));
 
       if (is_avg_pool) {
-        const bool count_exclude_pad = bool(helper.Get("count_include_pad", int64_t(0))) == false;
+        const bool count_exclude_pad = helper.Get("count_include_pad", int64_t(0)) == 0;
         AddOperationInput(*op, "exclude_padding_from_average",
                           model_builder.AddScalarConstant(op->type(), "count_exclude_pad", count_exclude_pad));
       }

onnxruntime/core/providers/coreml/builders/impl/resize_op_builder.cc

@@ -184,14 +184,16 @@ bool ResizeOpBuilder::IsOpSupportedImpl(const Node& node, const OpBuilderInputPa
 
     bool using_scales = input_defs.size() >= 3 && input_defs[2]->Exists();
     // scales
-    if (using_scales && !Contains(initializers, input_defs[2]->Name())) {
+    if (using_scales && !input_params.graph_viewer.GetConstantInitializer(input_defs[2]->Name())) {
       LOGS(logger, VERBOSE) << "scales input of Resize must be a constant initializer";
       return false;
     }
 
     // sizes
     if (!using_scales &&
-        (input_defs.size() < 4 || !input_defs[3]->Exists() || !Contains(initializers, input_defs[3]->Name()))) {
+        (input_defs.size() < 4 ||
+         !input_defs[3]->Exists() ||
+         !input_params.graph_viewer.GetConstantInitializer(input_defs[3]->Name()))) {
       LOGS(logger, VERBOSE) << "sizes input of Resize must be a constant initializer";
       return false;
     }

onnxruntime/core/providers/coreml/builders/model_builder.cc

@@ -622,43 +622,43 @@ const std::string& ModelBuilder::AddTensorValueAsConstantOperation(std::string_v
 }
 
 template <typename T>
-const std::string& ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type, gsl::span<const T> value,
-                                                 std::optional<gsl::span<const int64_t>> shape) {
+std::string_view ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
+                                               gsl::span<const T> value,
+                                               std::optional<gsl::span<const int64_t>> shape) {
   // add specialization below
   static_assert(false_for_T<T>, "Missing specialization for value type");
 
-  static const std::string error_message = "ModelBuilder::AddConstant error";
-  return error_message;  // unreachable
+  return "ModelBuilder::AddConstant error";  // unreachable
 }
 
 template <>
-const std::string& ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
-                                                 gsl::span<const float> value,
-                                                 std::optional<gsl::span<const int64_t>> shape) {
+std::string_view ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
+                                               gsl::span<const float> value,
+                                               std::optional<gsl::span<const int64_t>> shape) {
   auto input_value = CreateTensorValue<float>(value, shape);
   return AddTensorValueAsConstantOperation(op_type, value_type, std::move(input_value));
 }
 
 template <>
-const std::string& ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
-                                                 gsl::span<const int64_t> value,
-                                                 std::optional<gsl::span<const int64_t>> shape) {
+std::string_view ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
+                                               gsl::span<const int64_t> value,
+                                               std::optional<gsl::span<const int64_t>> shape) {
   auto input_value = CreateTensorValue<int64_t, int32_t>(value, shape);  // CoreML uses int32
   return AddTensorValueAsConstantOperation(op_type, value_type, std::move(input_value));
 }
 
 template <>
-const std::string& ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
-                                                 gsl::span<const bool> value,
-                                                 std::optional<gsl::span<const int64_t>> shape) {
+std::string_view ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
+                                               gsl::span<const bool> value,
+                                               std::optional<gsl::span<const int64_t>> shape) {
   auto input_value = CreateTensorValue<bool>(value, shape);
   return AddTensorValueAsConstantOperation(op_type, value_type, std::move(input_value));
 }
 
 template <>
-const std::string& ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
-                                                 gsl::span<const std::string> value,
-                                                 std::optional<gsl::span<const int64_t>> shape) {
+std::string_view ModelBuilder::AddConstantImpl(std::string_view op_type, std::string_view value_type,
+                                               gsl::span<const std::string> value,
+                                               std::optional<gsl::span<const int64_t>> shape) {
   auto input_value = CreateTensorValue<std::string>(value, shape);
   return AddTensorValueAsConstantOperation(op_type, value_type, std::move(input_value));
 }

onnxruntime/core/providers/coreml/builders/model_builder.h

@@ -104,8 +104,8 @@ class ModelBuilder {
   /// </param>
   /// <returns>Unique name generated for value.</returns>
   template <typename T>
-  const std::string& AddConstant(std::string_view op_type, std::string_view value_type, gsl::span<const T> value,
-                                 std::optional<gsl::span<const int64_t>> shape = std::nullopt) {
+  std::string_view AddConstant(std::string_view op_type, std::string_view value_type, gsl::span<const T> value,
+                               std::optional<gsl::span<const int64_t>> shape = std::nullopt) {
     static_assert(std::is_same_v<T, float> ||
                       std::is_same_v<T, int64_t> ||
                       std::is_same_v<T, std::string> ||
@@ -116,26 +116,19 @@ class ModelBuilder {
   }
 
   template <typename T>
-  const std::string& AddConstant(std::string_view op_type, std::string_view value_type, const std::vector<T>& value,
-                                 std::optional<gsl::span<const int64_t>> shape = std::nullopt) {
+  std::string_view AddConstant(std::string_view op_type, std::string_view value_type, const std::vector<T>& value,
+                               std::optional<gsl::span<const int64_t>> shape = std::nullopt) {
     return AddConstant(op_type, value_type, AsSpan(value), shape);
   }
 
   /// <summary>
   /// Add a scalar value as a 'const' operation. See AddConstant for details.
   /// </summary>
   template <typename T>
-  const std::string& AddScalarConstant(std::string_view op_type, std::string_view value_type, const T& value) {
+  std::string_view AddScalarConstant(std::string_view op_type, std::string_view value_type, const T& value) {
     return AddConstant(op_type, value_type, AsSpan({value}), AsSpan<const int64_t>({}));
   }
 
-  /// <summary>
-  /// Add an existing a constant ONNX initializer to the ML Program as a 'const' operation
-  /// </summary>
-  /// <param name="name">Initializer name</param>
-  /// <param name="initializer">Initializer data</param>
-  void AddConstant(std::string_view name, const ONNX_NAMESPACE::TensorProto& initializer);
-
   // add the operation to the main function
   void AddOperation(std::unique_ptr<COREML_SPEC::MILSpec::Operation> operation);
 #endif
@@ -160,8 +153,8 @@ class ModelBuilder {
  private:
 #if defined(COREML_ENABLE_MLPROGRAM)
   template <typename T>
-  const std::string& AddConstantImpl(std::string_view op_type, std::string_view value_type, gsl::span<const T> value,
-                                     std::optional<gsl::span<const int64_t>> shape = std::nullopt);
+  std::string_view AddConstantImpl(std::string_view op_type, std::string_view value_type, gsl::span<const T> value,
+                                   std::optional<gsl::span<const int64_t>> shape = std::nullopt);
 
   // apply the CoreML naming rules and fix any invalid names.
   const std::string& GetSafeName(const std::string& name);

onnxruntime/core/providers/coreml/model/host_utils.h

@@ -48,6 +48,11 @@
 #define HAS_COREML6_OR_LATER @available(macOS 13, iOS 16, *)
 #define HAS_COREML7_OR_LATER @available(macOS 14, iOS 17, *)
 
+#if !defined(NDEBUG)
+// Override location the model is written to so that a) it's easily found and b) it is not automatically deleted
+// when the EP exits.
+constexpr const char* DEBUG_MODEL_DIRECTORY = "ORT_COREML_EP_MODEL_DIR";
+#endif
 #endif
 
 #define MINIMUM_COREML_VERSION 3            // first version we support

onnxruntime/core/providers/coreml/model/host_utils.mm

@@ -34,7 +34,7 @@ int32_t CoreMLVersion() {
   NSURL* temporary_directory_url = [NSURL fileURLWithPath:NSTemporaryDirectory() isDirectory:YES];
 
 #if !defined(NDEBUG)
-  std::string path_override = Env::Default().GetEnvironmentVar("ORT_COREML_EP_MODEL_DIR");
+  std::string path_override = Env::Default().GetEnvironmentVar(DEBUG_MODEL_DIRECTORY);
   if (!path_override.empty()) {
     NSString* ns_path_override = [NSString stringWithUTF8String:path_override.c_str()];
     temporary_directory_url = [NSURL fileURLWithPath:ns_path_override isDirectory:YES];

onnxruntime/core/providers/coreml/model/model.mm

@@ -289,7 +289,7 @@ - (void)cleanup {
   }
 
 #if !defined(NDEBUG)
-  std::string path_override = Env::Default().GetEnvironmentVar("ORT_COREML_EP_MODEL_DIR");
+  std::string path_override = Env::Default().GetEnvironmentVar(DEBUG_MODEL_DIRECTORY);
   if (!path_override.empty()) {
     // don't cleanup
     coreml_model_path_ = nil;

onnxruntime/test/perftest/command_args_parser.cc

@@ -120,7 +120,7 @@ namespace perftest {
       "\t    [Example] [For NNAPI EP] -e nnapi -i \"NNAPI_FLAG_USE_FP16 NNAPI_FLAG_USE_NCHW NNAPI_FLAG_CPU_DISABLED\"\n"
       "\n"
       "\t    [CoreML only] [COREML_FLAG_CREATE_MLPROGRAM]: Create an ML Program model instead of Neural Network.\n"
-      "\t    [Example] [For CoreML EP] -e cormel -i \"COREML_FLAG_CREATE_MLPROGRAM\"\n"
+      "\t    [Example] [For CoreML EP] -e coreml -i \"COREML_FLAG_CREATE_MLPROGRAM\"\n"
       "\n"
       "\t    [SNPE only] [runtime]: SNPE runtime, options: 'CPU', 'GPU', 'GPU_FLOAT16', 'DSP', 'AIP_FIXED_TF'. \n"
       "\t    [SNPE only] [priority]: execution priority, options: 'low', 'normal'. \n"