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

Aarch64 jit depthwise convolution kernels #2014

Conversation

nishith-fujitsu
Copy link
Contributor

Description

This commit expands ARM SVE support for forward and backward JIT SVE Depth-wise convolution in FP32 for, introducing compatibility with various vector lengths. The changes made are for implementing different ARM ISA.
Major code changes:

  1. Added common files jit_uni_dw_convolution.cpp, jit_uni_dw_convolution.hpp, jit_uni_dw_conv_kernel_utils.hpp, jit_uni_dw_conv_kernel_f32.cpp, jit_uni_dw_conv_kernel_f32.hpp to accommodate the extended ARM SVE ISA for depth-wise convolution operators.
  2. Set data format tags according to the ARM ISA being used for forward and backward operator in JIT depth-wise convolution.
  3. Replaced ldr, and str instructions for vector registers with ld1w and st1w to utilize predication.
  4. Made changes to reducer.cpp to support Vector agnostic approach.

Checklist

General

  • [✓] Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit? Yes
    Test output is same with and without this commit.
99% tests passed, 2 tests failed out of 196
 
Total Test time (real) = 525.25 sec
 
The following tests FAILED:
        155 - test_graph_unit_dnnl_large_partition_usm_cpu (Failed)
        177 - test_benchdnn_modeC_graph_ci_cpu (Failed)
Errors while running CTest
Output from these tests are in: /home/nishith/oss/oneDNN/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
make: *** [Makefile:71: test] Error 8
  • [ ✓] Have you formatted the code using clang-format? Yes

@vpirogov vpirogov added this to the v3.6 milestone Jul 29, 2024
@vpirogov vpirogov added the platform:cpu-aarch64 Codeowner: @oneapi-src/onednn-cpu-aarch64 label Jul 29, 2024
@vpirogov vpirogov changed the base branch from main to vpirogov/codeowners-update July 31, 2024 16:27
@vpirogov vpirogov requested review from a team as code owners July 31, 2024 16:27
@vpirogov vpirogov changed the base branch from vpirogov/codeowners-update to main July 31, 2024 16:27
@vpirogov vpirogov removed the request for review from a team July 31, 2024 16:27
@abhijain1204fujitsu
Copy link

@vpirogov , Can you please share your feedback for the PR & Support us for merger

@vpirogov
Copy link
Contributor

vpirogov commented Aug 6, 2024

@oneapi-src/onednn-cpu-aarch64, @snadampal, @kawakami-k, could you please help reviewing and validating this one?

Copy link
Contributor

@jondea jondea left a comment

Choose a reason for hiding this comment

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

This is really great work, thank you!

My only important comment is that I think it is better than the current ACL depthwise implementation and it should go ahead of it in the dispatch list.

I have also done some testing with this on a C7g with benchdnn and with PyTorch, and I didn't hit any issues.

Copy link
Contributor

@jondea jondea left a comment

Choose a reason for hiding this comment

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

Code generally looks good, but can you remove the merge commit from the history by rebasing onto main please? https://github.com/oneapi-src/oneDNN/blob/main/CONTRIBUTING.md#code-contribution-guidelines

@nishith-fujitsu nishith-fujitsu force-pushed the AARCH64_JIT_Depthwise_Convolution_kernels branch 2 times, most recently from 7740689 to ad33f1c Compare August 19, 2024 10:23
@abhijain1204fujitsu
Copy link

@vpirogov , @jondea the requested changes has been completed, kindly check and support to merge the PR

@abhijain1204fujitsu
Copy link

@vpirogov kindly support to merge the changes
In case there is any more feedback kindly let us know.
Thank you !

Copy link
Contributor

@mgouicem mgouicem left a comment

Choose a reason for hiding this comment

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

I believe a missing closing brace should make that code break the build on ARM platforms.

@nishith-fujitsu nishith-fujitsu force-pushed the AARCH64_JIT_Depthwise_Convolution_kernels branch from 8dd253d to 9a4fcc5 Compare August 26, 2024 09:42
Copy link
Contributor

@mgouicem mgouicem left a comment

Choose a reason for hiding this comment

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

please remove the merge commit. Instead rebase your branch on top of the main branch.

@nishith-fujitsu nishith-fujitsu force-pushed the AARCH64_JIT_Depthwise_Convolution_kernels branch from e70a447 to 9a4fcc5 Compare August 26, 2024 09:48
@nishith-fujitsu nishith-fujitsu force-pushed the AARCH64_JIT_Depthwise_Convolution_kernels branch from 9a4fcc5 to 6f6a7ae Compare August 26, 2024 09:55
@nishith-fujitsu
Copy link
Contributor Author

please remove the merge commit. Instead rebase your branch on top of the main branch.

Hi @mgouicem, removed merge commits and rebased to latest main branch.

@abhijain1204fujitsu
Copy link

@vpirogov , @mgouicem please support to merge the PR

@vpirogov vpirogov merged commit 5639fae into uxlfoundation:main Aug 26, 2024
10 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:cpu-aarch64 Codeowner: @oneapi-src/onednn-cpu-aarch64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants