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

graph: backend: dnnl: encode mem address into constant cache key #2312

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

xiang1guo
Copy link
Contributor

@xiang1guo xiang1guo commented Dec 24, 2024

Description

Same compiled partitions may have different constant weights, potential accuracy issue may happens on future op direct optimization integration solutions.

This PR aims to enhance the library constant cache key for better cache or differentiation.

For the future direction of API design, please refer to #2280 for details. We will revisit this RFC once the real user scenario and request pops out.

Performance impact

  • Pattern level performance impact
    • CPU: No impact
    • GPU: No impact
  • Model level performance impact
    • IPEX model:
      • RN50 fp32 no impact.
      • Bert large fp32/bf16/int8 no impact.

@xiang1guo xiang1guo added the component:graph-api Codeowner: @oneapi-src/onednn-graph label Dec 24, 2024
@xiang1guo xiang1guo self-assigned this Dec 24, 2024
@xiang1guo xiang1guo requested a review from a team as a code owner December 24, 2024 03:11
@github-actions github-actions bot added the component:tests Codeowner: @oneapi-src/onednn-arch label Dec 24, 2024
@xiang1guo xiang1guo force-pushed the xiang/main/fix-constant-cache-key branch from f4499e2 to ac702db Compare December 24, 2024 04:42
@xiang1guo xiang1guo force-pushed the xiang/main/fix-constant-cache-key branch from ac702db to 89d402e Compare December 25, 2024 06:14
@xiang1guo
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

@@ -43,6 +43,24 @@ bool kernel_base_t::enabled_constant_cache() const {
return enabled;
}

size_t kernel_base_t::encode_constant_cache_key(
const std::vector<tensor_t> &inputs, size_t cache_key) const {
// Encode the constant memory address into cache key for differentiation
Copy link
Contributor

Choose a reason for hiding this comment

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

With this, the original constant_key_ is not a cache key anymore. I would suggest to rename these variables as well as the function name of generate_constant_cache_key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! Rename to const_md_hash_ and generate_constant_md_hash accordingly, please review again.

src/graph/backend/dnnl/kernels/kernel_base.cpp Outdated Show resolved Hide resolved
src/graph/backend/dnnl/kernels/kernel_base.cpp Outdated Show resolved Hide resolved
src/graph/backend/dnnl/kernels/large_partition.cpp Outdated Show resolved Hide resolved
@xiang1guo xiang1guo force-pushed the xiang/main/fix-constant-cache-key branch from 470f190 to 28606ed Compare December 31, 2024 03:14
@xiang1guo
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:graph-api Codeowner: @oneapi-src/onednn-graph component:tests Codeowner: @oneapi-src/onednn-arch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants