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 : reorder : zp correction for 4d shapes #2224

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Shreyas-fuj
Copy link
Contributor

Description

This PR is the fix for the issue : #2213 raised on github.
There was a missing condition that had to be added when zero-points are involved in the reorder.

Checklist

General

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

99% tests passed, 2 tests failed out of 218

Total Test time (real) = 1695.47 sec

The following tests FAILED:
	167 - test_graph_unit_dnnl_large_partition_cpu (Failed)
	199 - test_benchdnn_modeC_graph_ci_cpu (Failed)
Errors while running CTest
Output from these tests are in: /home/shreyas/G/shr-fuj/oneDNN_open_source/build/Testing/Temporary/LastTest.log
Use "--rerun-failed --output-on-failure" to re-run the failed cases verbosely.
make: *** [Makefile:71: test] Error 8
  • [y] Have you formatted the code using clang-format?

Copy link
Contributor

@Ryo-not-rio Ryo-not-rio left a comment

Choose a reason for hiding this comment

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

Thank you for creating a fix quickly, could you also add the below test to tests/benchdnn/inputs/reorder/test_reorder_ci?


# 4d reorders
--sdt=s8,u8
--ddt=f32
--attr-zero-points=src:common:-1
--stag=adbc
1x12x128x33

@@ -791,7 +791,7 @@ struct jit_uni_reorder_kernel_f32_t : public kernel_t, public jit_generator {
// transposition on the fly
const bool fast_return = prb_.src_scale_type != scale_type_t::MANY
&& prb_.dst_scale_type != scale_type_t::MANY
&& prb_.beta == 0.f;
&& prb_.beta == 0.f && !prb_.req_src_zp && !prb_.req_dst_zp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, do we need !prb_.req_dst_zp here? In my experimentation the dst zero point wasn't causing issues but I may not have found the right shapes which would cause an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ryo-not-rio , thanks for the review.
So this looks like it was initially ported from x86 jit_uni_reorder. When I compared assemblies of the 2 I figured this was the point at which it was behaving differently. So for now I have used the same condition used in x86. We can do further testing in future to remove the condition for !prb_.req_dst_zp . But for now I feel its safe to keep it. Anyway this would still fall to jit:uni but just a fast_return feature would be disabled.

@Shreyas-fuj
Copy link
Contributor Author

Thank you for creating a fix quickly, could you also add the below test to tests/benchdnn/inputs/reorder/test_reorder_ci?


# 4d reorders
--sdt=s8,u8
--ddt=f32
--attr-zero-points=src:common:-1
--stag=adbc
1x12x128x33

Sure, I have added the above test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants