generic:sycl: Inner Product Backward #2360

AD2605 · 2025-01-09T07:33:13Z

Description

Adds the backward_data and the backward_weight of the inner product for the generic backend and supports all possible layouts of inputs / outputs. To enable the same, this PR also enables the forward pass supporting different possible layouts of inputs / outputs, which was previously missing.

Checklist

General

Do all unit and benchdnn tests (make test and make test_benchdnn_*) pass locally for each commit?
Have you formatted the code using clang-format?

ranukund

A couple of edits suggested. Please incorporate as you see fit, thanks!

src/gpu/generic/sycl/README.md

src/gpu/nvidia/cudnn_matmul_impl.hpp

AD2605 · 2025-01-17T11:37:43Z

The Failure in [CI AArch64 / c7g, gcc, OMP, Release (pull_request)](https://github.com/oneapi-src/oneDNN/actions/runs/12812746484/job/35725156241?pr=2360)
seem to be unrelated to the changes made by this PR (They are CPU failures on Aarch64)

209/209 Test #187: test_benchdnn_modeC_conv_ci_cpu .........................***Failed 1872.65 sec

vpirogov · 2025-01-17T16:31:48Z

The Failure in [CI AArch64 / c7g, gcc, OMP, Release (pull_request)](https://github.com/oneapi-src/oneDNN/actions/runs/12812746484/job/35725156241?pr=2360) seem to be unrelated to the changes made by this PR (They are CPU failures on Aarch64)

209/209 Test #187: test_benchdnn_modeC_conv_ci_cpu .........................***Failed 1872.65 sec

This is a flaky test. Issue tracked in #2303.

mgouicem

The test part seems fine by me, but please correct the propagation kind checks in implementation.

Less important things:

For init functions, please don't hesitate to use the VDISPATCH macros, as those help debug and analyse user issues (you can find an example here).
Regarding implementation in header file, could you move as much of those as possible to cpp file?

src/gpu/generic/sycl/ref_inner_product.hpp

mgouicem · 2025-01-20T08:26:41Z

make test
disable build_vendor_intel
disable build_vendor_intel
disable test_device_cpu
enable build_vendor_generic
enable arch_gpu_ampere
enable compiler_icx-oss

AD2605 · 2025-02-03T13:15:00Z

For init functions, please don't hesitate to use the VDISPATCH macros, as those help debug and analyse user issues (you can find an example [here](https://github.com/oneapi-src/oneDNN/blob/611f2fecc87d89cc618496c5149acd062ba8c68f/src/cpu/ref_inner_product.hpp

This was a good suggestion, thanks @mgouicem .

Regarding implementation in header file, could you move as much of those as possible to cpp file?

Moved almost everything to the cpp file

AD2605 · 2025-02-04T10:23:09Z

make test
disable build_vendor_intel
disable build_vendor_intel
disable test_device_cpu
enable build_vendor_generic
enable arch_gpu_ampere
enable compiler_icx-oss

AD2605 · 2025-02-05T10:38:32Z

make test
disable build_vendor_intel
disable build_vendor_intel
disable test_device_cpu
enable build_vendor_generic
enable arch_gpu_ampere
enable compiler_icx-oss

AD2605 · 2025-02-11T10:35:40Z

make test
disable build_vendor_intel
disable build_vendor_intel
disable test_device_cpu
enable build_vendor_generic
enable arch_gpu_ampere
enable compiler_icx-oss

AD2605 · 2025-02-11T10:36:21Z

@oneapi-src/onednn-arch gentle ping.

AD2605 · 2025-02-11T10:42:28Z

The CI failures related to Inner product are because the pd_init returns unimplemented.
PS: Benchdnn also seems to test datatype combinations which are not supported by spec, for example u8:s8:f16, which is not supported acc to spec

AD2605 · 2025-02-12T10:07:59Z

I can confirm that the CI failures are unrelated to the inner product implementation.
The IP "fails" for the generic backend because unimplemented are being treated as failed at the moment.
That should be remedied by #2614

dzarukin · 2025-02-13T00:46:18Z

src/gpu/generic/sycl/ref_inner_product.cpp

+            assert(!"wrong number of dimensions for inner product");
+            return format_tag::undef;
+    }
+}


dnnl::impl::get_abx_tag() does the same. Please reuse.

dzarukin · 2025-02-13T00:48:20Z

src/gpu/generic/sycl/ref_inner_product.cpp

+        std::shared_ptr<primitive_desc_t> &pd) {
+
+    primitive_desc_iterator_t it(engine, op_desc, attributes, nullptr);
+    if (!it.is_initialized()) return status::invalid_arguments;


Please restore out_of_memory error status. It's used everywhere for this condition.

Apologies forgot to reply to this, this was covered as a part of the latest commit

It feels the statuses got flipped.

if (!initialized) return status::out_of_memory; ... if (!assigned) return status::unimplemented;

aha yes, corrected it now

dzarukin · 2025-02-13T01:07:03Z

src/gpu/generic/sycl/ref_inner_product.cpp

+    const bool ok = (set_default_params() == status::success);
+    if (!ok) { return status::unimplemented; }


VDISPATCH_INNER_PRODUCT support?

using VDISPATCH_INNER_PRODUCT now

src/gpu/generic/sycl/ref_inner_product.cpp

dzarukin · 2025-02-13T01:13:41Z

src/gpu/generic/sycl/ref_inner_product.cpp

+                    = detail::get_cfirst_format_from_ndims(wei_wrapper.ndims());
+        }
+    } else {
+        if (!src_needs_reorder


!src_needs_reorder is secured with else.

Missed replying to this, this was addressed in one of the previous commits

dzarukin · 2025-02-13T01:14:32Z

src/gpu/generic/sycl/ref_inner_product.cpp

+
+                    case 5: wei_reordered_tag = format_tag::acdeb; break;
+                    default:
+                        assert(!"Wrong number of dimensions for inner product");


dnnl::impl::get_axb_format() does the same. Please reuse.

AD2605 · 2025-03-03T12:25:39Z

updated with the latest main, to avoid compilation issues.
@dzarukin @mgouicem gentle ping for reviews / approvals

dzarukin · 2025-03-03T17:25:58Z

src/gpu/generic/sycl/ref_inner_product.cpp

+        if (*it) {
+            assigned = true;
+            pd = *it;
+            break;


You could just return status::success here without adding a new variable.

dzarukin · 2025-03-03T17:26:00Z

src/gpu/generic/sycl/ref_inner_product.cpp

+        std::shared_ptr<primitive_desc_t> &pd) {
+
+    primitive_desc_iterator_t it(engine, op_desc, attributes, nullptr);
+    if (!it.is_initialized()) return status::invalid_arguments;


It feels the statuses got flipped.

if (!initialized) return status::out_of_memory; ... if (!assigned) return status::unimplemented;

dzarukin · 2025-03-03T17:36:06Z

src/gpu/generic/sycl/ref_inner_product.cpp

+            && src_strides[0] == src_reshaped.dims[1];
+    bool is_not_strided = ((src_strides[0] == 1 || src_strides[1] == 1)
+            && src_wrapper.ndims() == 2); // check for strided plain layouts
+    is_favourable_layout = is_favourable_layout & is_not_strided;


It's not a bitwise logic or operation.

Suggested change

is_favourable_layout = is_favourable_layout & is_not_strided;

is_favourable_layout = is_favourable_layout && is_not_strided;

dzarukin · 2025-03-03T17:38:29Z

src/gpu/generic/sycl/ref_inner_product.cpp

+        auto src_reorder_scratchpad
+                = ctx.get_scratchpad_grantor().get_memory_storage(
+                        memory_tracking::names::key_iprod_src_reorder);
+        // An md should not be required to simply access the scratchpad storage


Is this comment relevant? There's one more like this below.

Yes, it's to signify that we are passing a dummy memory descriptor so that we can simply extract the pointer, and a valid memory descriptor is not required.
I added the comment to avoid any confusion in the future in case someone else is dealing with this section of the code

AD2605 · 2025-03-04T15:39:02Z

Since this PR has the required approvals and I have addressed all the comment, I will be going ahead and merging this PR

AD2605 requested review from a team as code owners January 9, 2025 07:33

ranukund reviewed Jan 9, 2025

View reviewed changes

src/gpu/generic/sycl/README.md Outdated Show resolved Hide resolved

src/gpu/generic/sycl/README.md Outdated Show resolved Hide resolved

Rbiessy reviewed Jan 10, 2025

View reviewed changes

src/gpu/generic/sycl/README.md Outdated Show resolved Hide resolved

src/gpu/nvidia/cudnn_matmul_impl.hpp Outdated Show resolved Hide resolved

sgeor255 approved these changes Jan 14, 2025

View reviewed changes

AD2605 force-pushed the svet/inner_product_bwd branch from 88fa721 to ac134aa Compare January 16, 2025 16:00

github-actions bot removed the platform:gpu-nvidia Codeowner: @oneapi-src/onednn-gpu-nvidia label Jan 16, 2025

Rbiessy approved these changes Jan 16, 2025

View reviewed changes

mgouicem reviewed Jan 20, 2025

View reviewed changes

src/gpu/generic/sycl/ref_inner_product.hpp Outdated Show resolved Hide resolved

src/gpu/generic/sycl/ref_inner_product.hpp Outdated Show resolved Hide resolved

src/gpu/generic/sycl/ref_inner_product.hpp Outdated Show resolved Hide resolved

AD2605 force-pushed the svet/inner_product_bwd branch from 4bce05e to aca98ca Compare February 3, 2025 13:10

AD2605 requested a review from mgouicem February 3, 2025 13:15

AD2605 force-pushed the svet/inner_product_bwd branch from aca98ca to aad22a9 Compare February 5, 2025 10:38

AD2605 force-pushed the svet/inner_product_bwd branch from aad22a9 to 8ab19b9 Compare February 11, 2025 10:35

dzarukin reviewed Feb 13, 2025

View reviewed changes

AD2605 force-pushed the svet/inner_product_bwd branch from 8ab19b9 to 1d0b87c Compare February 24, 2025 12:00

AD2605 requested a review from dzarukin February 28, 2025 16:20

AD2605 force-pushed the svet/inner_product_bwd branch from 1d0b87c to e5852cb Compare March 3, 2025 12:23

dzarukin approved these changes Mar 3, 2025

View reviewed changes

gpu: generic: inner product bwd

e8c9745

AD2605 force-pushed the svet/inner_product_bwd branch from e5852cb to e8c9745 Compare March 4, 2025 14:11

AD2605 merged commit de97197 into uxlfoundation:main Mar 4, 2025
22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

generic:sycl: Inner Product Backward #2360

generic:sycl: Inner Product Backward #2360

AD2605 commented Jan 9, 2025 •

edited

Loading

ranukund left a comment

AD2605 commented Jan 17, 2025

vpirogov commented Jan 17, 2025

mgouicem left a comment

mgouicem commented Jan 20, 2025

AD2605 commented Feb 3, 2025

AD2605 commented Feb 4, 2025

AD2605 commented Feb 5, 2025

AD2605 commented Feb 11, 2025

AD2605 commented Feb 11, 2025

AD2605 commented Feb 11, 2025

AD2605 commented Feb 12, 2025

dzarukin Feb 13, 2025

AD2605 Feb 24, 2025

dzarukin Feb 13, 2025

AD2605 Feb 26, 2025

dzarukin Mar 3, 2025

AD2605 Mar 4, 2025

dzarukin Feb 13, 2025

AD2605 Feb 24, 2025

dzarukin Feb 13, 2025

AD2605 Mar 4, 2025

dzarukin Feb 13, 2025

AD2605 Feb 24, 2025

AD2605 commented Mar 3, 2025

dzarukin Mar 3, 2025

AD2605 Mar 4, 2025

dzarukin Mar 3, 2025

dzarukin Mar 3, 2025

AD2605 Mar 4, 2025

dzarukin Mar 3, 2025

AD2605 Mar 4, 2025

AD2605 commented Mar 4, 2025

		const bool ok = (set_default_params() == status::success);
		if (!ok) { return status::unimplemented; }

	is_favourable_layout = is_favourable_layout & is_not_strided;
	is_favourable_layout = is_favourable_layout && is_not_strided;

generic:sycl: Inner Product Backward #2360

generic:sycl: Inner Product Backward #2360

Conversation

AD2605 commented Jan 9, 2025 • edited Loading

Description

Checklist

General

ranukund left a comment

Choose a reason for hiding this comment

AD2605 commented Jan 17, 2025

vpirogov commented Jan 17, 2025

mgouicem left a comment

Choose a reason for hiding this comment

mgouicem commented Jan 20, 2025

AD2605 commented Feb 3, 2025

AD2605 commented Feb 4, 2025

AD2605 commented Feb 5, 2025

AD2605 commented Feb 11, 2025

AD2605 commented Feb 11, 2025

AD2605 commented Feb 11, 2025

AD2605 commented Feb 12, 2025

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

AD2605 commented Mar 3, 2025

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

AD2605 commented Mar 4, 2025

AD2605 commented Jan 9, 2025 •

edited

Loading