Skip to content

graph: handle different engine instances in compied parition cache #3413

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

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

Conversation

gyhintel
Copy link
Contributor

Fix MFDNN-12121
In this PR, we add the reset_engine method to reset the compiled partition's and kernel's engine. With this PR, the compiled partition and primitive cache can be hit for different engine cases (Same member functions and member variables)

@gyhintel gyhintel self-assigned this Jun 11, 2025
@gyhintel gyhintel requested a review from a team as a code owner June 11, 2025 08:33
@gyhintel gyhintel added the component:graph-api Codeowner: @oneapi-src/onednn-graph label Jun 11, 2025
@github-actions github-actions bot added the component:tests Codeowner: @oneapi-src/onednn-arch label Jun 11, 2025
@gyhintel gyhintel force-pushed the gyh/main/cp_eigine branch from ea29874 to 2ff64a5 Compare June 11, 2025 08:47
@gyhintel
Copy link
Contributor Author

make test
set test_scope=NIGHTLY
disable benchdnn_all
enable benchdnn_graph

@@ -97,6 +97,10 @@ struct sdp_primitive_kernel_t : public kernel_base_t {

DEF_KERNEL_METHOD_STR(sdp_primitive_kernel_t)
DNNL_DISALLOW_COPY_AND_ASSIGN(sdp_primitive_kernel_t)
status_t reset_engine(const engine_t *g_engine) override {
UNUSED(g_engine);
Copy link
Contributor

Choose a reason for hiding this comment

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

why unused here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sdp_primitive_kernel_t, the executable object is primitive_tL70. For primitive_t object, there is no engine in this object. So we don't need to reset engine for it.

@gyhintel gyhintel force-pushed the gyh/main/cp_eigine branch from 2ff64a5 to 6ebe88b Compare June 17, 2025 06:38
@gyhintel
Copy link
Contributor Author

make test
set test_scope=NIGHTLY
disable benchdnn_all
enable benchdnn_graph

@@ -113,7 +113,7 @@ status_t dnnl_partition_impl_t::compile(
compiled_partition_t *compiled_partition,
const std::vector<logical_tensor_t> &inputs,
const std::vector<logical_tensor_t> &outputs,
const engine_t *g_engine) const {
engine_t *g_engine) const {
Copy link
Contributor

Choose a reason for hiding this comment

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

What does this mean? We do have a const engine in the public compile API. I assume that const-ness will propagate from API to internal implementations.

@@ -91,6 +91,7 @@ struct batch_norm_fwd_t : public kernel_base_t {

DEF_KERNEL_METHOD_STR(batch_norm_fwd_t)
DNNL_DISALLOW_COPY_AND_ASSIGN(batch_norm_fwd_t)
KERNEL_RESET_ENGINE
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
KERNEL_RESET_ENGINE
DEF_KERNEL_METHOD_RESET_ENGINE

Just try to align with the name of DEF_KERNEL_METHOD_STR.

@gyhintel gyhintel force-pushed the gyh/main/cp_eigine branch from 6ebe88b to 06880e9 Compare June 18, 2025 07:03
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.

3 participants