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

test: aarch64: Skip Jit uni reorder if 4d matrix and zero point is defined #2207

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

Conversation

Ryo-not-rio
Copy link
Contributor

Description

Jit:uni reorders for certain 4d matrices are returning incorrect results when the src zero point != 0. A guard has been added to skip jit uni reorders for any 4d matrices where src zero point != 0 so the simple:any implementation will be used. A new testcase has also been added to benchdnn.

Reproduction:

./tests/benchdnn/benchdnn --reorder  --sdt=u8 --ddt=f32 --stag=adbc --attr-zero-points=src:common:1 1x32x128x33

General

  • Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit?
  • Have you formatted the code using clang-format?

Bug fixes

  • Have you included information on how to reproduce the issue (either in a github issue or in this PR)?
  • Have you added relevant regression tests?

@Ryo-not-rio Ryo-not-rio requested review from a team as code owners November 7, 2024 16:06
bool has_zero_point
= !_pd->attr()->zero_points_.has_default_values(DNNL_ARG_FROM)
|| !_pd->attr()->zero_points_.has_default_values(DNNL_ARG_TO);
if (src_md->ndims == 4 && has_zero_point) { return status::unimplemented; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this implementation is a copy-paste of existing one for x64, I suggest moving the check either into prb_init or into tr::kernel_t::desc_init.

As for the check itself, is it only ndims=4? Or any ndims is affected by this limitation?

If the latter, then existing testing coverage should be enough to trigger the error and probably might not be needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the review, I have moved the check to prb_init.

As far as I can tell, the issue seems to be only ndims=4. I have run test_benchdnn_modeC_reorder_ci_cpu which passes so I would assume that the other ndims are okay

@github-actions github-actions bot added the platform:cpu-aarch64 Codeowner: @oneapi-src/onednn-cpu-aarch64 label Nov 7, 2024
@Ryo-not-rio Ryo-not-rio changed the title Skip Jit uni reorder if 4d matrix and zero point is defined test: Skip Jit uni reorder if 4d matrix and zero point is defined Nov 8, 2024
…fined

Change-Id: I837a8f66d8e8f6afce8e944416db96987e95abac
@Ryo-not-rio Ryo-not-rio changed the title test: Skip Jit uni reorder if 4d matrix and zero point is defined test: aarch64: Skip Jit uni reorder if 4d matrix and zero point is defined Nov 12, 2024
@Shreyas-fuj
Copy link
Contributor

Shreyas-fuj commented Nov 20, 2024

Hi @Ryo-not-rio, @theComputeKid, @dzarukin
Can you please hold this PR as we have found the bug fix for this and will be raising a PR shortly so that the above mentioned cases passes on jit:uni itself instead of falling back to simple:any.
Thanks.

@theComputeKid
Copy link
Contributor

@Shreyas-fuj Thanks for looking into it. Much appreciated.

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.

4 participants