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.

Sign up for GitHub

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

Jump to bottom

graph: backend: kernels: sdp verbose enhancement #2273

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

graph: backend: kernels: sdp verbose enhancement #2273

wants to merge 1 commit into from
+66 −41

Conversation

rongzha1
Copy link
Contributor

[Graph API] Code quality improvement: enhancement for SDP

@rongzha1 rongzha1 requested a review from a team as a code owner December 16, 2024 08:43
@rongzha1
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

TaoLv
TaoLv reviewed Dec 17, 2024
View reviewed changes
Copy link
Contributor

@TaoLv TaoLv left a comment

Choose a reason for hiding this comment

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

Please share some verbose examples for these checks.

src/graph/backend/dnnl/kernels/sdp_decomp.hpp Outdated
@@ -163,6 +163,7 @@ struct sdp_decomp_kernel_t : public kernel_base_t {
UNUSED(outputs);
UNUSED(sycl_deps);
UNUSED(sycl_event);
VINFO(graph, exec, check, sdp, "no sycl execution for decomp sdp ");
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel we don't need these VINFO check in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed. thanks for your advice

@rongzha1
Copy link
Contributor Author

Please share some verbose examples for these checks.

for dispatch:

onednn_verbose,v1,graph,create:dispatch,compile,dispatch to sdp_decomp_kernel,src/graph/backend/dnnl/kernels/sdp.hpp:79
onednn_verbose,v1,graph,create:dispatch,compile,dispatch to larger_partition_kernel,src/graph/backend/dnnl/kernels/sdp.hpp:86

for check:

onednn_verbose,v1,graph,create:check,sdp_decomp,Not support select between mm1 and scale,src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp:460
onednn_verbose,v1,graph,create:check,sdp_decomp,Failed to record input offset,src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp:33
onednn_verbose,v1,graph,create:check,sdp_decomp,sdp_decomp_kernel_t: initial check failed,src/graph/backend/dnnl/kernels/sdp_decomp.cpp:56

@rongzha1
Copy link
Contributor Author

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

TaoLv
TaoLv reviewed Dec 18, 2024
View reviewed changes
src/graph/backend/dnnl/kernels/sdp.hpp
@@ -65,17 +68,25 @@ struct sdp_base_t : public kernel_base_t {
if (enable_ukernel) {
kernel = std::make_shared<sdp_primitive_kernel_t<quantized>>();
ret = kernel->compile_impl(part, g_engine, inputs, outputs);
if (ret == status::success)
VDISPATCH_GRAPH_SDP("dispatch to sdp_primitive_kernel");
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
VDISPATCH_GRAPH_SDP("dispatch to sdp_primitive_kernel");
VDISPATCH_GRAPH_SDP("dispatch to sdp_primitive_kernel_t");

Just in case you intention is to show the class name.

src/graph/backend/dnnl/kernels/sdp.hpp
}

if (ret != status::success && enable_decomp) {
kernel = std::make_shared<sdp_decomp_kernel_t<quantized, dt>>();
ret = kernel->compile_impl(part, g_engine, inputs, outputs);
if (ret == status::success)
VDISPATCH_GRAPH_SDP("dispatch to sdp_decomp_kernel");
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, sdp_decomp_kernel_t

src/graph/backend/dnnl/kernels/sdp_decomp.cpp
return status::unimplemented;
VCONDCHECK(graph, create, check, sdp_decomp,
sdp_cfg_.initial_check(subgraph_, inputs), status::unimplemented,
"sdp_decomp_kernel_t: initial check failed");
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need this as we already do checks in the initial_check function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TaoLv TaoLv TaoLv left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
component:graph-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rongzha1 @TaoLv