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

cpu: aarch64: Expand ARM SVE support for 1x1 convolution #2075

Merged
merged 1 commit into from
Oct 3, 2024

Conversation

rpushkarr
Copy link
Contributor

Description

This pull request introduces the implementation of Just-In-Time (JIT) 1x1 forward and backward convolution for block size 8. The following changes and updates have been made:

  • Block Size Level Changes: Modifications are made to accommodate the new block size 8 requirements.
  • Tag Additions: New tags are introduced to support and identify the changes related to block size 8.
  • cpu_convolution_list Update: The cpu_convolution_list is updated to include the new JIT 1x1 convolution implementations for the same.

Furthermore, additional conditions have been added to ensure compatibility and execution multiple ISAs by incorporating relevant changes

Checklist

General

  • [ ✓] Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit? Yes

make test output:
99% tests passed, 2 tests failed out of 199

Total Test time (real) = 1512.99 sec

The following tests FAILED:
158 - test_graph_unit_dnnl_large_partition_usm_cpu (Failed)
180 - test_benchdnn_modeC_graph_ci_cpu (Failed)
Errors while running CTest
Output from these tests are in: /home/ravip/files/JIT/1x1bwd/oss_pr/oneDNN/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
make: *** [Makefile:71: test] Error

  • [✓ ] Have you formatted the code using clang-format? Yes

@rpushkarr rpushkarr requested review from a team as code owners September 3, 2024 12:10
@vpirogov vpirogov added the platform:cpu-aarch64 Codeowner: @oneapi-src/onednn-cpu-aarch64 label Sep 3, 2024
@vpirogov vpirogov added this to the v3.6 milestone Sep 3, 2024
@abhijain1204fujitsu
Copy link

@vpirogov , @jondea Kindly support us to review the PR

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.

Thanks for the PR. Tagging @jondea and @kawakami-k for additional review.

From my side:

  • please add new tags at the end, to prevent ABI breakage
  • please fix the commit message to adhere to the project convention. Something like cpu: aarch64: add 1x1-convolution support for block size 8 should do.
  • just to make sure: did you run scripts/generate_dnnl_debug.py to modify *debug_autogenerated.cpp files?

include/oneapi/dnnl/dnnl_types.h Outdated Show resolved Hide resolved
@vpirogov vpirogov modified the milestones: v3.6, v3.7 Sep 9, 2024
@abhijain1204fujitsu
Copy link

@mgouicem , @jondea kindly support to review the changes

@mgouicem
Copy link
Contributor

@abhijain1204fujitsu we are still waiting for the previous comments to be addressed (new tags moved to end of list to avoid ABI breakage, commit naming, and proper generation of debug info)

@theComputeKid
Copy link
Contributor

Can you please confirm that no new compiler warnings are emitted after your change?

@rpushkarr
Copy link
Contributor Author

Hi @vpirogov ,

I synced the repo to resolve merge conflicts in dnnl_types.h and cpu_convolution_list.cpp, but the files in my branch were deleted, and the PR shows as merged and closed. However, the updates aren't reflected in the "main" branch, and the other changes were reverted. Could you help me reopen the PR and resolve this issue?

Thanks!

@vpirogov
Copy link
Member

@rpushkarr, looks like something went wrong during the sync. Github shows a force push into your branch that dropped all changes from your branch resulting in automatic closure of the pull request:
image

You may try recovering your changes from commit da03e2f. Once you have changes on your branch you will have an option to reopen the PR.

@rpushkarr rpushkarr reopened this Sep 23, 2024
@rpushkarr
Copy link
Contributor Author

@mgouicem @theComputeKid @vpirogov
I have made all the necessary changes and re-opened the PR. Could you review the same?

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.

Thanks for the changes. LGTM on the common/API part.
By any chance, would you have performance data you could share?
Tagging @jondea @kawakami-k for aach64 part.

@theComputeKid
Copy link
Contributor

@Radu2k can you please look into this for aarch64.

@theComputeKid
Copy link
Contributor

/Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/aarch64/jit_brgemm_conv_comp_pad_kernel.hpp:46:47: warning: template-id not allowed for constructor in C++20 [-Wtemplate-id-cdtor]
   46 |     jit_uni_brgemm_conv_comp_pad_kernel_t<isa>(
      |                                               ^
/Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/aarch64/jit_brgemm_conv_comp_pad_kernel.hpp:46:47: note: remove the '< >'
In file included from /Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/aarch64/jit_sve_1x1_convolution.hpp:34,
                 from /Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/cpu_convolution_list.cpp:62:
/Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/aarch64/jit_uni_1x1_conv_utils.hpp:164:23: warning: template-id not allowed for constructor in C++20 [-Wtemplate-id-cdtor]
  164 |     rtus_driver_t<isa>(int iw, int stride_w, int src_step_h, int src_step_icb,

Seems like warnings are coming from this file according to the aarch64 ci. Can you please fix them before merging. Thanks.

@rpushkarr
Copy link
Contributor Author

@theComputeKid
Could you kindly assist me in reproducing this warning, as I am not encountering it on my system?

@theComputeKid
Copy link
Contributor

@rpushkarr : the aarch64 team has since fixed all compiler warnings as part of #2122, so you don't have to worry about that anymore. What you can do is to rebase your changes to trigger the pipelines we have since added to ensure build & tests pass.

@rpushkarr
Copy link
Contributor Author

@jondea @theComputeKid
I have rebased the changes. Kindly review it.

@theComputeKid
Copy link
Contributor

@rpushkarr : Our CI has detected that you have added compiler warnings, and as we have Werror turned on, the CI pipeline has failed. You can find the warnings here: https://github.com/oneapi-src/oneDNN/actions/runs/11121825321/job/30901786505?pr=2075

Please fix the code to remove the warning so we can proceed, as we can not merge until the CI passes. Thanks!

In file included from /Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/aarch64/jit_sve_1x1_conv_kernel.cpp:35:
/Users/runner/work/oneDNN/oneDNN/oneDNN/src/cpu/aarch64/jit_uni_1x1_conv_utils.hpp:164:23: error: template-id not allowed for constructor in C++20 [-Werror=template-id-cdtor]
  164 |     rtus_driver_t<isa>(int iw, int stride_w, int src_step_h, int src_step_icb,
      |                       ^

@rpushkarr
Copy link
Contributor Author

@jondea @theComputeKid
I have rebased the changes. Please take a moment to review them.

Copy link
Contributor

@theComputeKid theComputeKid left a comment

Choose a reason for hiding this comment

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

Thanks!

@spalicki spalicki merged commit 47b2b9b into oneapi-src:main Oct 3, 2024
23 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.

6 participants