[CoreML MLProgram] Support Float16 (1/N) #22068
Conversation
wejoncy
force-pushed
the
jicwen/coreml_fp16
branch
from
e134133
to
305e9ab
Compare
onnxruntime/core/providers/coreml/builders/impl/builder_utils.cc
Outdated
Show resolved
Hide resolved
| @@ -257,6 +257,98 @@ TEST(CoreMLExecutionProviderTest, TestNameSanitization) { | |||
| // TensorRT does not support Clip opset 11 yet. | |||
| test.Run(OpTester::ExpectResult::kExpectSuccess, "", {kTensorrtExecutionProvider}); | |||
| } | |||
|
|
|||
| TEST(CoreMLExecutionProviderTest, TestBinaryFp16) { | |||
There was a problem hiding this comment.
Do we need to add separate tests here or could we update onnxruntime\test\providers\cpu\math\element_wise_ops_test.cc to run the MLFloat16 tests it has for CoreML?
We're also going to add some xnnpack fp16 kernels so the more common test code that is used the better.
There was a problem hiding this comment.
Good idea. Refactored some of elemenwise UTs. It's still a bit messy for FP16/Bf16 test.
wejoncy
force-pushed
the
jicwen/coreml_fp16
branch
from
305e9ab
to
3944fd6
Compare
wejoncy
force-pushed
the
jicwen/coreml_fp16
branch
from
957c4d9
to
4f935e7
Compare
onnxruntime/core/providers/coreml/builders/impl/builder_utils.cc
Outdated
Show resolved
Hide resolved
onnxruntime/core/providers/coreml/builders/impl/builder_utils.h
Outdated
Show resolved
Hide resolved
onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc
Outdated
Show resolved
Hide resolved
onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc
Outdated
Show resolved
Hide resolved
onnxruntime/core/providers/coreml/builders/impl/gemm_op_builder.cc
Outdated
Show resolved
Hide resolved
| test.AddAttribute("beta", 1.0f); | ||
| test.AddInput<MLFloat16>("A", {2, 4}, f_A); | ||
| test.AddInput<MLFloat16>("B", {4, 3}, f_B, true); | ||
| f_C.resize(3); |
There was a problem hiding this comment.
nit: create a new local variable so that a developer doesn't have to track the changes made to f_C across multiple different tests.
would also be good to capture in a comment what exactly is being tested by using different input sizes for f_C and why.
may also be good to use something other than all 1's for the bias input as that could potentially hide an issue in the handling.
| @@ -37,6 +37,12 @@ void TestConvFp16Op(const ConvOpAndTestAttributes& attributes, | |||
| OpTester::ExpectResult expect_result = OpTester::ExpectResult::kExpectSuccess, | |||
| const std::string& err_str = "", | |||
| int opset = 11) { | |||
| #if !defined(MLAS_F16VEC_INTRINSICS_SUPPORTED) | |||
| // a `return` after tester will make binary crash | |||
There was a problem hiding this comment.
Is the reason that if you do not have MLAS_F16VEC_INTRINSICS_SUPPORTED there's no support for the custom NhwcFusedConv attribute? And now that we hit this code when the CoreML EP is enabled we need to do an early exit in that case as no EP can handle the node?
There was a problem hiding this comment.
I think I misunderstand it. I just need to exclude coremlEP when it has activation fused.
There was a problem hiding this comment.
My question was more about making the comment explain 'why' rather than 'what' will happen. If you say 'it will crash' the next developer to look at the code has to start from scratch to figure out the reason in order to decide if this applies to their potential changes not.
I think it comes down to the activation attribute being something that the FusedConv/NhwcFusedConv operators have that the ONNX Conv doesn't. As the CoreML EP doesn't support those contrib ops, if the CPU EP can't handle it due to the lack of MLAS support the OpTester won't find any EPs that can run and fails.
i.e. if attributes.activation is not empty we're using a contrib op that only a small number of EPs support.
I'm surprised this check isn't also required for the QNN EP as I don't see FusedConv or NhwcFusedConv in the ops it supports.
There was a problem hiding this comment.
Got it.
Comments added
Co-authored-by: Scott McKay <[email protected]>
Co-authored-by: Scott McKay <[email protected]>
…r.cc Co-authored-by: Scott McKay <[email protected]>
Co-authored-by: Scott McKay <[email protected]>
Co-authored-by: Scott McKay <[email protected]>
wejoncy
force-pushed
the
jicwen/coreml_fp16
branch
from
47de692
to
fe3a3a3
Compare
wejoncy
force-pushed
the
jicwen/coreml_fp16
branch
from
c3fc5bb
to
48c98ec
Compare
Description
Support Float16 for CoreML MLProgram EP.
Operations:
Unary/Binary/Act/pool/shape/gemm/conv
Motivation and Context