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

x64: pool: Accuracy issue in benchnn pooling test #2678

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

Conversation

jstachowintel
Copy link

It is similar use case as #2627. The problem is with huge input sizes, so I decided disable src_size <= INT_MAX for jit:avx512_core implementation.

@github-actions github-actions bot added the platform:cpu-x64 Intel64/AMD64 processors. Codeowner: @oneapi-src/onednn-cpu-x64 label Feb 12, 2025
@jstachowintel jstachowintel changed the title checking if src_size is overflowed x64: pool: Accuracy issue in benchnn pooling test Feb 12, 2025
@@ -75,6 +75,13 @@ struct jit_uni_pooling_fwd_t : public primitive_t {
CHECK(jit_uni_pool_kernel<isa>::init_conf(
jpp_, scratchpad, attr_, this));

dim_t src_size = static_cast<dim_t>(jpp_.mb) * jpp_.c * jpp_.id
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to type cast? Is this check just going to fail if we happen to get problem dimensions exceeding INT_MAX?

Copy link
Author

Choose a reason for hiding this comment

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

If the product of our input sizes exceeds INT_MAX, an overflow will occur, and we want to skip this implementation. We need to use casting to get the correct, not overflowed number that can be compared with INT_MAX.

Copy link
Contributor

@rjoursler rjoursler Feb 13, 2025

Choose a reason for hiding this comment

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

This issue is that jpp_.mb may have already overflowed. As I understand this code, all the problem dimensions are set here. The src_d memory descriptor returns a dim_t which is truncated for storing in jpp. If we set one of those dimensions to something like jpp.mb UINT_MAX + 1, then jpp_.mb will be 1 and the check may inadvertently pass. It seem better to just directly check the memory descriptor sizes similar to here or to change the dimensions types in jpp to dim_t so that no cast is necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform:cpu-x64 Intel64/AMD64 processors. Codeowner: @oneapi-src/onednn-cpu-x64
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants