From d866225bc3dd4d43296f6e76bbb6b26a0c5734b8 Mon Sep 17 00:00:00 2001 From: Scott McKay Date: Sat, 20 Jul 2024 12:42:39 +1000 Subject: [PATCH] Address PR comments --- .../builders/impl/convtranspose_op_builder.cc | 10 ++++---- .../core/providers/coreml/model/model.h | 2 +- .../core/providers/coreml/model/model.mm | 23 ++++++++++--------- .../test/providers/coreml/utils_test.mm | 2 +- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/onnxruntime/core/providers/coreml/builders/impl/convtranspose_op_builder.cc b/onnxruntime/core/providers/coreml/builders/impl/convtranspose_op_builder.cc index db33240b8a7c..eca996597f76 100644 --- a/onnxruntime/core/providers/coreml/builders/impl/convtranspose_op_builder.cc +++ b/onnxruntime/core/providers/coreml/builders/impl/convtranspose_op_builder.cc @@ -106,7 +106,7 @@ bool ConvTransposeOpBuilder::IsOpSupportedImpl(const Node& node, const OpBuilder // - output_shape CoreML output is inconsistent so disabled for now // // NOTE: need to test with/without the COREML_FLAG_USE_CPU_ONLY flag being set to get an idea of how flaky the CoreML - // behaviour is. + // behavior is. // Update /onnxruntime/test/util/default_providers.cc:DefaultCoreMLExecutionProvider to do so const auto& input_defs = node.InputDefs(); @@ -154,13 +154,14 @@ bool ConvTransposeOpBuilder::IsOpSupportedImpl(const Node& node, const OpBuilder LOGS(logger, VERBOSE) << "ConvTranspose: support for SAME_LOWER/SAME_UPPER is not implemented yet"; return false; } else if (autopad == AutoPadType::NOTSET) { - // CoreML output is inconsistent if pads are asymmetric. - // CPU works. Other devices don't seem to (at least on macOS). + // CoreML output is inconsistent between CPU_ONLY and ALL if the pads aren't all the same value. + // CPU matches the expected output, but other devices don't seem to (at least on macOS). auto onnx_pads = *helper.GetInt64s("pads"); // 'pads' are required if auto_pad is NOTSET const auto pad_value = onnx_pads[0]; if (!std::all_of(onnx_pads.begin() + 1, onnx_pads.end(), [pad_value](auto value) { return value == pad_value; })) { - LOGS(logger, VERBOSE) << "ConvTranspose: pads must be symmetric for CoreML to return consistent results"; + LOGS(logger, VERBOSE) << "ConvTranspose: all pad values must be the same for CoreML to return " + "consistent results"; return false; } } @@ -193,7 +194,6 @@ bool ConvTransposeOpBuilder::IsOpSupportedImpl(const Node& node, const OpBuilder // ONNX. Running without that flag produces the expected output. Madness... auto output_shape = helper.GetInt64s("output_shape"); if (output_shape) { - // there is an output_shape input, but the padding seems to be different so results don't LOGS(logger, VERBOSE) << "ConvTranspose: output_shape is not supported as the CoreML output is inconsistent"; return false; } diff --git a/onnxruntime/core/providers/coreml/model/model.h b/onnxruntime/core/providers/coreml/model/model.h index 5bce7cf996e4..5ea0521d973f 100644 --- a/onnxruntime/core/providers/coreml/model/model.h +++ b/onnxruntime/core/providers/coreml/model/model.h @@ -42,7 +42,7 @@ using GetOutputTensorMutableRawDataFn = std::function&)inputs int64_t block_size = 0; int64_t stride = 0; - ORT_RETURN_IF_ERROR(GetMLMultiArrayCopyInfo(data, &num_blocks, &block_size, &stride)); + ORT_RETURN_IF_ERROR(GetMLMultiArrayCopyInfo(data, num_blocks, block_size, stride)); __block Status copy_status; const auto* tensor_info = &output_tensor_info; @@ -454,7 +454,8 @@ - (Status)predict:(const std::unordered_map&)inputs namespace onnxruntime { namespace coreml { -Status GetMLMultiArrayCopyInfo(const MLMultiArray* array, int64_t* num_blocks, int64_t* block_size, int64_t* stride) { +Status GetMLMultiArrayCopyInfo(const MLMultiArray* _Nonnull array, + int64_t& num_blocks, int64_t& block_size, int64_t& stride) { const auto* shape = array.shape; const auto rank = shape.count; @@ -466,13 +467,13 @@ Status GetMLMultiArrayCopyInfo(const MLMultiArray* array, int64_t* num_blocks, i int64_t this_stride = [array.strides[rank - i] longLongValue]; if (this_stride != total_elems) { // non-contiguous if we have to move more than batch_elems for each entry - if (*block_size != 0) { + if (block_size != 0) { return ORT_MAKE_STATUS(ONNXRUNTIME, FAIL, "Multiple non-contiguous dimensions in MLMultiArray are not supported."); } - *block_size = data_elems; - *stride = this_stride; + block_size = data_elems; + stride = this_stride; } const auto elems_this_dim = [shape[rank - i] longLongValue]; @@ -480,17 +481,17 @@ Status GetMLMultiArrayCopyInfo(const MLMultiArray* array, int64_t* num_blocks, i total_elems = elems_this_dim * this_stride; } - if (*block_size == 0) { + if (block_size == 0) { // contiguous - *block_size = data_elems; - *stride = array_total_elements; + block_size = data_elems; + stride = array_total_elements; } - *num_blocks = data_elems / *block_size; + num_blocks = data_elems / block_size; ORT_ENFORCE(array_total_elements == total_elems, "Logic error calculating copy info"); - ORT_ENFORCE(*stride >= *block_size, "Logic error calculating copy info"); - ORT_ENFORCE(*stride * *num_blocks == total_elems, "Logic error calculating copy info"); + ORT_ENFORCE(stride >= block_size, "Logic error calculating copy info"); + ORT_ENFORCE(stride * num_blocks == total_elems, "Logic error calculating copy info"); return Status::OK(); } diff --git a/onnxruntime/test/providers/coreml/utils_test.mm b/onnxruntime/test/providers/coreml/utils_test.mm index 2ea3d229c0d8..8e0fc779467f 100644 --- a/onnxruntime/test/providers/coreml/utils_test.mm +++ b/onnxruntime/test/providers/coreml/utils_test.mm @@ -19,7 +19,7 @@ auto ValidateGetInfo(MLMultiArray* array, int64_t num_blocks = 0; int64_t block_size = 0; int64_t stride = 0; - auto status = coreml::GetMLMultiArrayCopyInfo(array, &num_blocks, &block_size, &stride); + auto status = coreml::GetMLMultiArrayCopyInfo(array, num_blocks, block_size, stride); if (!expect_valid) { ASSERT_STATUS_NOT_OK(status);