graph: backend: kernels: sdp verbose enhancement #2273

rongzha1 · 2024-12-16T08:43:32Z

[Graph API] Code quality improvement: enhancement for SDP

rongzha1 · 2024-12-16T08:45:31Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

TaoLv

Please share some verbose examples for these checks.

src/graph/backend/dnnl/kernels/sdp_decomp.hpp

rongzha1 · 2024-12-18T07:19:42Z

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 · 2024-12-18T07:22:19Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

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

src/graph/backend/dnnl/kernels/sdp_decomp.cpp

rongzha1 · 2024-12-19T02:59:14Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

rongzha1 · 2024-12-20T01:26:05Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

rongzha1 · 2024-12-20T01:26:26Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

rongzha1 · 2024-12-24T02:01:53Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

gyhintel

Should also add some check on L68, L81, L438, L58, L127, L245, L278 ?

rongzha1 · 2024-12-24T08:39:22Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

rongzha1 · 2024-12-24T08:43:10Z

Thanks for your comments. L68, L81, L438 fixed, L58, L127, L245, L278 keep them to align primitive style.
Could you review it again ? @gyhintel

TaoLv · 2024-12-25T05:40:38Z

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

        }
+        if (ret != status::success)
+            VDISPATCH_GRAPH_SDP("fail to dispatch to sdp kernel");


I'm thinking to simplify the checks. Is it possible to do the check only before return?

if (ret == status::success) VDISPATCH_GRAPH_SDP("sdpa is dispatched to (%s)", kernel->str()); else VDISPATCH_GRAPH_SDP("sdpa is failed to dispatch");

Good idea. changed as suggested

TaoLv · 2024-12-25T05:43:36Z

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp

-    auto op_status = record_input_offset(sg, inputs);
-    if (op_status != status::success) return false;
+    VCHECK_SDP_DECOMP(record_input_offset(sg, inputs) == status::success, false,
+            "Failed to record input offset");


Any specific consideration to wrap the record_input_offset() function into the macro? I feel the below style is easier to read and to step into the function when debug.

auto op_status = record_input_offset(sg, inputs); VCHECK_SDP_DECOMP(op_status == status::success, false, "Failed to record input offset");

keep the same style as primitive. Since check with msg is already added in record_input_offset
will change it to

CHECK_BOOL(record_input_offset(sg, inputs));

TaoLv · 2024-12-25T05:44:51Z

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp

-    }
+    VCHECK_SDP_DECOMP(
+            batch_size == wei1_user_dims[0] && batch_size == wei2_user_dims[0],
+            false, "Batch size mismatch");


Do we want to also print the values of them?

just added in print

TaoLv · 2024-12-25T05:47:38Z

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp


    // Check scale size
    if (graph_inport[2] != -1) {
        auto scale_sz = ltw(inputs[graph_inport[2]]).nelems();
-        if (scale_sz != 1) return false;
+        VCHECK_SDP_DECOMP(scale_sz == 1, false,
+                "Scale size in sdp should be 1, but got %d", scale_sz);


Better to clarify that this is a limitation of the decomp kernel, not sdp.

Suggested change

"Scale size in sdp should be 1, but got %d", scale_sz);

"sdp_decomp_kernel_t only supports single scale value, but got %d", scale_sz);

Okay i see you already have kernel name in the macro, but seems still need to refine. This is a duplication in your example:

onednn_verbose,v1,graph,create:check,sdp_decomp,sdp_decomp_kernel_t: initial check failed,src/graph/backend/dnnl/kernels/sdp_decomp.cpp:56

has removed the duplicated info. thanks for your advice.

TaoLv · 2024-12-25T05:48:28Z

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp

-#else
-    return true;
+    VCHECK_SDP_DECOMP(batch_size * num_head_q > RATIO * nthr, false,
+            "Batch size * num_head_q should be larger than %d * nthr", RATIO);


This is also a kernel limitation. I'm not sure if the message clarifies that.

added more info.

TaoLv · 2024-12-25T05:50:33Z

src/graph/backend/dnnl/kernels/sdp_primitive_config.cpp

@@ -19,6 +19,10 @@

 #include "common/compiler_workarounds.hpp"

+#define VCHECK_SDP_PRIMITIVE(cond, status, msg, ...) \
+    VCONDCHECK(graph, create, check, sdp_primitive, (cond), status, msg, \


Suggested change

VCONDCHECK(graph, create, check, sdp_primitive, (cond), status, msg, \

VCONDCHECK(graph, create, check, sdp_primitive_kernel_t, (cond), status, msg, \

fixed. thanks

TaoLv · 2024-12-25T05:52:39Z

src/graph/backend/dnnl/kernels/sdp_primitive_config.cpp

-    }
-
+    VCONDCHECK(graph, create, dispatch, sdp, status == status::success, status,
+            "could not create primitive, falling back\n");


Suggested change

"could not create primitive, falling back\n");

"could not create sdpa primitive, falling back\n");

fixed. thanks

rongzha1 · 2024-12-25T09:24:02Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

rongzha1 · 2024-12-25T12:42:28Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp

rongzha1 · 2024-12-26T03:13:55Z

make test
enable benchdnn_nightly
disable benchdnn_all
enable benchdnn_graph

rongzha1 requested a review from a team as a code owner December 16, 2024 08:43

rongzha1 added the component:graph-api Codeowner: @oneapi-src/onednn-graph label Dec 16, 2024

TaoLv reviewed Dec 17, 2024

View reviewed changes

src/graph/backend/dnnl/kernels/sdp_decomp.hpp Outdated Show resolved Hide resolved

rongzha1 force-pushed the rzhang/verbose_sdp branch from b75abab to 9b8918d Compare December 18, 2024 07:21

TaoLv reviewed Dec 18, 2024

View reviewed changes

src/graph/backend/dnnl/kernels/sdp.hpp Outdated Show resolved Hide resolved

src/graph/backend/dnnl/kernels/sdp.hpp Outdated Show resolved Hide resolved

src/graph/backend/dnnl/kernels/sdp_decomp.cpp Outdated Show resolved Hide resolved

rongzha1 force-pushed the rzhang/verbose_sdp branch from 9b8918d to 5a9ac0b Compare December 19, 2024 02:55

rongzha1 force-pushed the rzhang/verbose_sdp branch from 5a9ac0b to 54d590e Compare December 20, 2024 01:26

rongzha1 force-pushed the rzhang/verbose_sdp branch from 54d590e to efe6e16 Compare December 24, 2024 01:34

gyhintel reviewed Dec 24, 2024

View reviewed changes

rongzha1 force-pushed the rzhang/verbose_sdp branch from efe6e16 to d19d49b Compare December 24, 2024 08:39

gyhintel approved these changes Dec 25, 2024

View reviewed changes

xiang1guo approved these changes Dec 25, 2024

View reviewed changes

TaoLv reviewed Dec 25, 2024

View reviewed changes

rongzha1 force-pushed the rzhang/verbose_sdp branch from d19d49b to 11a4780 Compare December 25, 2024 09:23

rongzha1 force-pushed the rzhang/verbose_sdp branch from 11a4780 to eb3d730 Compare December 25, 2024 12:21

ElaineBao reviewed Dec 26, 2024

View reviewed changes

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp Outdated Show resolved Hide resolved

src/graph/backend/dnnl/kernels/sdp_decomp_config.cpp Show resolved Hide resolved

graph: backend: kernels: sdp verbose enhancement

9bf40d3

rongzha1 force-pushed the rzhang/verbose_sdp branch from eb3d730 to 9bf40d3 Compare December 26, 2024 02:58

ElaineBao approved these changes Dec 26, 2024

View reviewed changes

TaoLv merged commit 68f22a4 into main Dec 26, 2024
13 checks passed

TaoLv deleted the rzhang/verbose_sdp branch December 26, 2024 09:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

graph: backend: kernels: sdp verbose enhancement #2273

graph: backend: kernels: sdp verbose enhancement #2273

rongzha1 commented Dec 16, 2024

rongzha1 commented Dec 16, 2024

TaoLv left a comment

rongzha1 commented Dec 18, 2024

rongzha1 commented Dec 18, 2024

rongzha1 commented Dec 19, 2024

rongzha1 commented Dec 20, 2024

rongzha1 commented Dec 20, 2024

rongzha1 commented Dec 24, 2024

gyhintel left a comment •

edited

Loading

rongzha1 commented Dec 24, 2024

rongzha1 commented Dec 24, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

TaoLv Dec 25, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

TaoLv Dec 25, 2024

rongzha1 Dec 25, 2024

rongzha1 commented Dec 25, 2024

rongzha1 commented Dec 25, 2024

rongzha1 commented Dec 26, 2024

	"Scale size in sdp should be 1, but got %d", scale_sz);
	"sdp_decomp_kernel_t only supports single scale value, but got %d", scale_sz);

	VCONDCHECK(graph, create, check, sdp_primitive, (cond), status, msg, \
	VCONDCHECK(graph, create, check, sdp_primitive_kernel_t, (cond), status, msg, \

	"could not create primitive, falling back\n");
	"could not create sdpa primitive, falling back\n");

graph: backend: kernels: sdp verbose enhancement #2273

graph: backend: kernels: sdp verbose enhancement #2273

Conversation

rongzha1 commented Dec 16, 2024

rongzha1 commented Dec 16, 2024

TaoLv left a comment

Choose a reason for hiding this comment

rongzha1 commented Dec 18, 2024

rongzha1 commented Dec 18, 2024

rongzha1 commented Dec 19, 2024

rongzha1 commented Dec 20, 2024

rongzha1 commented Dec 20, 2024

rongzha1 commented Dec 24, 2024

gyhintel left a comment • edited Loading

Choose a reason for hiding this comment

rongzha1 commented Dec 24, 2024

rongzha1 commented Dec 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rongzha1 commented Dec 25, 2024

rongzha1 commented Dec 25, 2024

rongzha1 commented Dec 26, 2024

gyhintel left a comment •

edited

Loading