[CoreML ] ML Program more operators support [3/N] #22710

wejoncy · 2024-11-04T08:32:56Z

Description

erf
Round
Max
ReduceMax
ReduceMean
ReduceSum
Unsqueeze
squeeze
Softmax

Motivation and Context

skottmckay · 2024-11-06T06:41:44Z

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

+    if (input_defs.size() > 2) {
+      std::string_view layer_input_name_x = model_builder.GetUniqueName(node, "variadic");
+      auto input_dtype = input_defs[0]->TypeAsProto()->tensor_type().elem_type();
+      const int32_t elem_type = static_cast<int32_t>(input_dtype);
+      std::vector<int64_t> x0_shape, x1_shape;
+      GetShape(*input_defs[0], x0_shape, logger);
+      GetShape(*input_defs[1], x1_shape, logger);
+      for (size_t i = 0; i < x0_shape.size(); i++) {
+        x0_shape[i] = std::max(x0_shape[i], x1_shape[i]);
+      }


Would be clearer if this explicitly called out if was specific to Max as well as a comment explaining how we're converting the variadic ONNX Max to the a sequence of CoreML 'maxiumum' operations.

Do we need to manually calculate the output shape? I would have expected CoreML could infer that.

Coreml Can't fully infer out the output shape
Output '0' has unexpected type 'ios18.maximum'. Expected tensor<fp32, [3, 3]>; got tensor<fp32, >

skottmckay · 2024-11-06T06:50:41Z

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

+    } else if (op_type == "ReduceSum") {
+      coreml_op_type = "reduce_sum";
+    } else if (op_type == "ReduceMean") {
+      coreml_op_type = "reduce_mean";
+    } else if (op_type == "ReduceMax") {
+      coreml_op_type = "reduce_max";


If it's trivial to add ReduceMin and ReduceProd for ML Program should we add those as well for completeness? #Closed

yeah, It's trival to support that. Will add it in this PR.

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

skottmckay · 2024-11-06T07:17:59Z

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

+  const auto* tensor_shape = node.InputDefs()[0]->Shape();
+  if (tensor_shape == nullptr) {
+    return false;
+  }


Do we need the shape to be known upfront if we're using the default start/end values?

Do we have a way to test it when shape-info is not exist?

If the prior node is a ConstantOfShape with a runtime input for the shape I don't think it would have shape info.

I thought ConastantFolding will resolve this.

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

skottmckay · 2024-11-06T08:42:56Z

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

+
+    if (coreml_op_type == "squeeze") {
+      if (!axes.empty()) {
+        AddOperationInput(*op, "axes", model_builder.AddConstant(op->type(), "axes", axes));


Do we need to handle negative axes here? #Closed

ML ops support negative axis
Will add comments above the statement.

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

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

onnxruntime/core/providers/cpu/tensor/unsqueeze.h

skottmckay · 2024-11-21T08:53:17Z

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

+// the conversion is doing the following:
+// max(x, y, z, ...) -> max(max(x, y), z, ...)
+#if defined(COREML_ENABLE_MLPROGRAM)
+static void AddVariadicInputsToMB(std::unique_ptr<CoreML::Specification::MILSpec::Operation>* op, ModelBuilder& model_builder, const Node& node,


nit: please keep lines to 120 chars

Would it be clearer to call this AddVariadicMax and have it handle all the work of adding the operator instead of doing part in AddToModelBuilderImpl and part here?

Min operator has the same schema, we can call it directly in case needing to support "Min"

skottmckay · 2024-11-21T08:58:11Z

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

@@ -86,8 +127,11 @@ Status BinaryOpBuilder::AddToModelBuilderImpl(ModelBuilder& model_builder, const
    std::unique_ptr<Operation> op = model_builder.CreateOperation(node, coreml_op_type);
    AddOperationInput(*op, "x", input_defs[0]->Name());
    AddOperationInput(*op, "y", input_defs[1]->Name());
+    if (input_defs.size() > 2) {


nit: I understand the check on inputs size is slightly more efficient, but explicitly checking for "Max" makes the code clearer as there's no question about which ops match the condition, and this isn't a performance critical path. The comment below helps, but comments can get out of date.

Suggested change

if (input_defs.size() > 2) {

if (op_type == "Max" && input_defs.size() > 2) {

skottmckay · 2024-11-21T09:05:29Z

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

+  GetShape(*input_defs[0], x0_shape, logger);
+  GetShape(*input_defs[1], x1_shape, logger);
+  if (x0_shape.size() < x1_shape.size()) {
+    std::swap(x0_shape, x1_shape);
+  }
+  std::fill(x0_shape.begin(), x0_shape.end(), -1);


Can we get the rank of each input instead of the shape?

Would be good to explain why we're adding a shape with -1 values in a comment so that's captured for the next developer that has to work on the code.

Yeah. I will add comments here to explain "-1".

skottmckay · 2024-11-21T09:10:46Z

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

+  const auto* tensor_shape = node.InputDefs()[0]->Shape();
+  if (tensor_shape == nullptr) {
+    return false;
+  }


If the prior node is a ConstantOfShape with a runtime input for the shape I don't think it would have shape info.

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

…lder.cc Co-authored-by: Scott McKay <[email protected]>

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

…der.cc Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

wejoncy requested a review from skottmckay November 4, 2024 08:33

wejoncy marked this pull request as ready for review November 4, 2024 08:33

wejoncy changed the title ~~[CoreML ] ML Program more operators support~~ [CoreML ] ML Program more operators support [3/N] Nov 4, 2024

wejoncy force-pushed the jicwen/coreml_ops branch from 8b444d3 to 43ac908 Compare November 4, 2024 08:39

wejoncy mentioned this pull request Nov 4, 2024

[Mobile] Supporting more operators for NNAPI and CoreML #22346

Open

more operators support

bbb09e9

wejoncy force-pushed the jicwen/coreml_ops branch from 43ac908 to bbb09e9 Compare November 4, 2024 08:47

wejoncy added 3 commits November 4, 2024 17:14

unused var

00fd39b

fix

d6bf630

fix

4c710c7

yuygfgg mentioned this pull request Nov 5, 2024

Add support for MLprogram in ort_coreml AmusementClub/vs-mlrt#116

Draft

skottmckay reviewed Nov 6, 2024

View reviewed changes

wejoncy force-pushed the jicwen/coreml_ops branch from 23b540b to ddd4b1e Compare November 8, 2024 12:23

github-actions bot reviewed Nov 8, 2024

View reviewed changes

address comments

7ba7d7b

wejoncy force-pushed the jicwen/coreml_ops branch from 2b23670 to 7ba7d7b Compare November 9, 2024 06:38

wejoncy requested a review from skottmckay November 20, 2024 09:11

skottmckay reviewed Nov 21, 2024

View reviewed changes

wejoncy and others added 2 commits November 21, 2024 17:43

Update onnxruntime/core/providers/coreml/builders/impl/squeeze_op_bui…

62516bc

…lder.cc Co-authored-by: Scott McKay <[email protected]>

address comments

f9002fd

github-actions bot reviewed Nov 21, 2024

View reviewed changes

onnxruntime/core/providers/coreml/builders/impl/binary_op_builder.cc Outdated Show resolved Hide resolved

wejoncy and others added 2 commits November 22, 2024 10:36

Update onnxruntime/core/providers/coreml/builders/impl/binary_op_buil…

d3e652d

…der.cc Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

simplify input_rank

3893f50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CoreML ] ML Program more operators support [3/N] #22710

[CoreML ] ML Program more operators support [3/N] #22710

wejoncy commented Nov 4, 2024 •

edited

Loading

skottmckay Nov 6, 2024

wejoncy Nov 8, 2024 •

edited

Loading

skottmckay Nov 6, 2024 •

edited

Loading

wejoncy Nov 8, 2024

skottmckay Nov 6, 2024

wejoncy Nov 8, 2024

skottmckay Nov 21, 2024

wejoncy Nov 21, 2024

skottmckay Nov 6, 2024 •

edited

Loading

wejoncy Nov 6, 2024 •

edited

Loading

skottmckay Nov 21, 2024

wejoncy Nov 21, 2024

skottmckay Nov 21, 2024 •

edited

Loading

skottmckay Nov 21, 2024

wejoncy Nov 21, 2024 •

edited

Loading

skottmckay Nov 21, 2024

	if (input_defs.size() > 2) {
	if (op_type == "Max" && input_defs.size() > 2) {

[CoreML ] ML Program more operators support [3/N] #22710

Are you sure you want to change the base?

[CoreML ] ML Program more operators support [3/N] #22710

Conversation

wejoncy commented Nov 4, 2024 • edited Loading

Description

Motivation and Context

Choose a reason for hiding this comment

wejoncy Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

skottmckay Nov 6, 2024 • edited Loading

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

Choose a reason for hiding this comment

skottmckay Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

wejoncy Nov 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skottmckay Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wejoncy Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wejoncy commented Nov 4, 2024 •

edited

Loading

wejoncy Nov 8, 2024 •

edited

Loading

skottmckay Nov 6, 2024 •

edited

Loading

skottmckay Nov 6, 2024 •

edited

Loading

wejoncy Nov 6, 2024 •

edited

Loading

skottmckay Nov 21, 2024 •

edited

Loading

wejoncy Nov 21, 2024 •

edited

Loading