[MKLDNN]Add quantized concat #13297
[MKLDNN]Add quantized concat #13297
Conversation
|
@mxnet-label-bot add [pr-awaiting-review] |
|
@larroy please help take a review again. This is an important OP for the quantization flow so we hope it can be merged before r1.4 code freeze. |
|
@zheng-da @reminisce @szha @eric-haibin-lin @apeforest |
| NNVM_REGISTER_OP(Concat) | ||
| .set_attr<FQuantizedOp>("FQuantizedOp", [](const NodeAttrs& attrs) { | ||
| nnvm::NodePtr node = nnvm::Node::Create(); | ||
| node->attrs.op = Op::Get("_contrib_quantized_concat"); |
There was a problem hiding this comment.
It seems the Concat operator will call the MKLDNN version of the operator. Is this intended?
There was a problem hiding this comment.
Yes, all quantized op doesn't have default implementation for cpu. MKLDNN is the only cpu implementation. I guess that's why they all have _contrib_ prefix.
There was a problem hiding this comment.
What if MKLDNN is not ON and user invokes this operator? Will any error message be given?
There was a problem hiding this comment.
I'm not sure. It should follow framework default behavior. Quantized op are all supported by FComputeEx, so basically it should only be used when MKLDNN is on. One way to avoid this is to define quantized_concat as a mkldnn specific op, by declaring it inside MXNET_USE_MKLDNN macro. But we don't have such backend specific op before. Do you have any suggestion?
There was a problem hiding this comment.
I think we should issue an error message that the quantized op is not supported in non MKLDNN build.
There was a problem hiding this comment.
Yes, I agree with that. But that beyond the scope of this PR. Currently all quantized op has this issue. We need to create another PR to add error message from framework level for each op that don't have default implementation.
There was a problem hiding this comment.
Can we have that PR first before we merge this? Knowing there is some limitation without issuing any clear message may create a bad user experience.
There was a problem hiding this comment.
@apeforest Build MXNet with make USE_OPENCV=1 USE_BLAS=openblas, and run quantized model, below message is reported:
[10:54:08] src/executor/attach_op_execs_pass.cc:351: Neither FCompute nor FComputeEx registered _contrib_quantized_concat
[10:54:08] src/executor/attach_op_execs_pass.cc:351: Neither FCompute nor FComputeEx registered _contrib_quantized_pooling
[10:54:08] src/executor/attach_op_execs_pass.cc:351: Neither FCompute nor FComputeEx registered _contrib_quantized_conv
I think framework can handle this case properly.
Codecov Report
@@ Coverage Diff @@
## master #13297 +/- ##
===========================================
- Coverage 79.72% 68.66% -11.07%
===========================================
Files 749 652 -97
Lines 81176 70894 -10282
Branches 3164 3164
===========================================
- Hits 64714 48676 -16038
- Misses 15606 21923 +6317
+ Partials 856 295 -561
Continue to review full report at Codecov.
|
|
@apeforest All comments are addressed. Can you review again? Thanks a lot for keeping review round after round. |
|
Thanks for the contribution. Now merging. |
Description
This PR is to add quantized concat op and its MKLDNN implementation.
@pengzhao-intel @TaoLv
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
Comments