Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[onert] Introduce DepthwiseConvOp in cpu kernel #13574

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

jyoungyun
Copy link
Contributor

This commit introduces DepthwiseConvOp in cpu kernel.
This kernel uses depthwise_conv and bias_op eigen functions.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun [email protected]

@jyoungyun jyoungyun added the PR/ready for review It is ready to review. Please review it. label Aug 1, 2024
@jyoungyun jyoungyun requested a review from a team August 1, 2024 08:00
@hseok-oh
Copy link
Contributor

hseok-oh commented Aug 2, 2024

In file included from /home/runner/work/ONE/ONE/runtime/onert/backend/cpu/ops/DepthwiseConvolutionLayer.cc:20:
In file included from /home/runner/work/ONE/ONE/compute/cker/include/cker/operation/DepthwiseConv.h:32:
/home/runner/work/ONE/ONE/compute/cker/include/cker/eigen/bias_op.h:100:7: error: unused variable 'channel_dim' [-Werror,-Wunused-variable]
  int channel_dim = input_shape.DimensionsCount() - 1;
      ^
1 error generated.
gmake[3]: *** [runtime/onert/backend/cpu/CMakeFiles/onert_backend_cpu.dir/build.make:272: runtime/onert/backend/cpu/CMakeFiles/onert_backend_cpu.dir/ops/DepthwiseConvolutionLayer.cc.o] Error 1

@jyoungyun jyoungyun added PR/NO MERGE Please don't merge. I'm still working on this :) and removed PR/ready for review It is ready to review. Please review it. labels Aug 2, 2024
@jyoungyun
Copy link
Contributor Author

Need #13582

This commit introduces DepthwiseConvOp in cpu kernel.
This kernel uses depthwise_conv and bias_op eigen functions.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
@jyoungyun jyoungyun added PR/ready for review It is ready to review. Please review it. and removed PR/NO MERGE Please don't merge. I'm still working on this :) labels Aug 5, 2024
ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
@jyoungyun
Copy link
Contributor Author

@ys44kim

there is no code that access bias_data, and it is passed as argument , there is no need to check for nullptr.
of course, even when there is no explicit nullptr input, sometimes the static analysis tool judges it to be an error when accessing the variable without checking for nullptr. of course, this does not apply in this case.
so, i think it is a good choice.

The code was changed to support optional bias data in DepthwiseConv2D cpu kernel. :)

i think it would be better to add a exception action for the case where bias_data is nullptr.
for example, add assert (bias_data) as follows refer to biasHelper,

In ONERT, the DepthwiseConv2D always has bias data. However, this behaviour can be changed because the DepthwiseConv2D does not necessarily require bias data.
So, I want to support optional bias in DepthwiseConv2D cpu kernel.
I added a test case for optional bias test case for verifying this cpu kernel.

Please take another look. :)

@ys44kim

This comment was marked as resolved.

@ys44kim
Copy link
Contributor

ys44kim commented Aug 8, 2024

@jyoungyun
i checked the commit history and review comment again and there was something I didn't understand well.
i will check more about the cpu kernel and review whether additional improvements are needed.
thank you for your kind reply.

@jyoungyun
Copy link
Contributor Author

jyoungyun commented Aug 8, 2024

@ys44kim

In onert the DepthwiseConv2D always has bias data (please tell me if this is wrong.)

Yes, you're right.

and, In fact, I don't understand why it is tested when bias is nullptr because onert does not support optional bias.

The codes under compute/cker directory is implemented using cpu for each operation.
ONERT uses various kinds of backends. Currently, ONERT takes cpu, acl_cl, neon, trix(npu) as backend. The behavior of each backend may or may not match ONERT.

In this case, ONERT does not support optional bias_data. However, CPU kernel that I imlemented on this PR supports optional bias_data because I think it is better to support more diverse functions in the cpu kernel. If ONERT supports optional bias in the future, we can support it without modifying this kernel.

Therefore, this cpu kernel has decided to support optional bias even though ONERT does not support it. :)

Copy link
Contributor

@ragmani ragmani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@ys44kim ys44kim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jyoungyun
Copy link
Contributor Author

@Samsung/one_onert Please merge this PR. :)

@hseok-oh hseok-oh merged commit 45fbbe2 into Samsung:master Aug 9, 2024
9 checks passed
@jyoungyun jyoungyun deleted the onert/add_depthwiseconvop branch August 12, 2024 05:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR/ready for review It is ready to review. Please review it.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants