[SYCL][E2E] Update get_coordinate tests to verify both row and columns #15880

YixingZhang007 · 2024-10-26T00:02:47Z

Improving Test Coverage and Cleanliness:

Implement validation for the get_coordinate function to check both row and column indices for all matrix types in get_coordinate_ops.cpp.
Remove the original tests: get_coord_int8_matA.cpp, get_coord_int8_matB.cpp, and get_coord_float_matC.cpp. These tests have been consolidated and are now included in get_coordinate_ops.cpp.

YixingZhang007 · 2024-10-26T00:34:21Z

This PR is still in progress. There might be some code clean up needed to be done.

uditagarwal97 · 2024-10-26T01:15:21Z

@YixingZhang007 I've updated the PR description to remove the JIRA link. The policy does not allow using internal JIRA links in open-source, only JIRA ticket numbers are allowed.

sycl/test-e2e/Matrix/get_coord_int8_matB_impl.hpp

dkhaldi · 2024-12-18T20:09:33Z

sycl/test-e2e/Matrix/get_coordinate_ops_impl.hpp

+
+  // This condition check can be removed once the IGC PR resolving the Matrix B
+  // row coordinate bug is pull downed to the driver.
+  if (Use != use::b) {


this is not needed anymore

dkhaldi

LGTM

dkhaldi · 2024-12-19T14:06:53Z

Ping @intel/llvm-reviewers-runtime

YixingZhang007 · 2024-12-19T14:10:10Z

@intel/llvm-gatekeepers This PR has been approved and all CI tests passed. Could you please help merge this PR? Thanks!

YuriPlyakhin · 2024-12-19T17:05:30Z

sycl/test-e2e/Matrix/get_coordinate_ops_impl.hpp

+           for (int i = 0; i < NUM_ROWS; i++) {
+             sum_local_rows[i] =
+                 reduce_over_group(sg, sum_local_rows[i], sycl::plus<>());
+             // only Groups leader perform the global reduction
+             if (global_idy % sg_size == 0) {
+               sycl::atomic_ref<TResult, sycl::memory_order::relaxed,
+                                sycl::memory_scope::device>
+                   aref(v_rows[i]);
+               aref.fetch_add(sum_local_rows[i]);
+             }
+           }
+
+           for (int i = 0; i < NUM_COLS; i++) {
+             sum_local_cols[i] =
+                 reduce_over_group(sg, sum_local_cols[i], sycl::plus<>());
+             // only Groups leader perform the global reduction
+             if (global_idy % sg_size == 0) {
+               sycl::atomic_ref<TResult, sycl::memory_order::relaxed,
+                                sycl::memory_scope::device>
+                   aref(v_cols[i]);
+               aref.fetch_add(sum_local_cols[i]);
+             }
+           }


@YixingZhang007 this code is duplicated. Would be great to outline it to function and reuse.

Thanks for the suggestion! I have created a PR for fixing this. The link to the PR is #16429

YuriPlyakhin · 2024-12-19T17:07:24Z

sycl/test-e2e/Matrix/get_coordinate_ops_impl.hpp

+  for (int i = 0; i < Rows; i++) {
+    for (int j = 0; j < Cols; j++) {
+      M[i][j] = i + j;
+    }
+  }


@YixingZhang007 , we have matrix_fill in common.hpp to not duplicate such loops in each test.

Thanks! I have changed this.

YuriPlyakhin · 2024-12-19T18:18:37Z

sycl/test-e2e/Matrix/get_coordinate_ops_impl.hpp

+                     {1, 1 * sg_size}),
+         [=](nd_item<2> spmd_item)
+#ifdef SG_SZ
+             [[intel::reqd_sub_group_size(SG_SZ)]]


attribute 'intel::reqd_sub_group_size' is deprecated

sycl::reqd_sub_group_size should be used

Sorry for the mistake! I have fix this in the other PR.

YixingZhang007 requested a review from a team as a code owner October 26, 2024 00:02

YixingZhang007 marked this pull request as draft October 26, 2024 00:02

YixingZhang007 self-assigned this Oct 26, 2024

YixingZhang007 temporarily deployed to WindowsCILock October 26, 2024 00:03 — with GitHub Actions Inactive

YixingZhang007 changed the title ~~Update get_coordinate tests to verify both row and columns for all matrix types~~ Update get_coordinate tests to verify both row and columns Oct 26, 2024

YixingZhang007 changed the title ~~Update get_coordinate tests to verify both row and columns~~ [SYCL][E2E] Update get_coordinate tests to verify both row and columns Oct 26, 2024

YixingZhang007 had a problem deploying to WindowsCILock October 26, 2024 00:20 — with GitHub Actions Error

YixingZhang007 had a problem deploying to WindowsCILock October 26, 2024 00:27 — with GitHub Actions Error

YixingZhang007 had a problem deploying to WindowsCILock October 26, 2024 00:28 — with GitHub Actions Error

YixingZhang007 had a problem deploying to WindowsCILock October 26, 2024 00:29 — with GitHub Actions Error

YixingZhang007 requested a review from dkhaldi October 26, 2024 00:33

YixingZhang007 temporarily deployed to WindowsCILock October 26, 2024 00:33 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 26, 2024 00:48 — with GitHub Actions Inactive

dkhaldi reviewed Oct 26, 2024

View reviewed changes

sycl/test-e2e/Matrix/get_coord_int8_matB_impl.hpp Outdated Show resolved Hide resolved

YixingZhang007 temporarily deployed to WindowsCILock October 26, 2024 17:40 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 26, 2024 17:54 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 29, 2024 22:36 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 29, 2024 22:57 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 30, 2024 22:16 — with GitHub Actions Inactive

YixingZhang007 had a problem deploying to WindowsCILock October 30, 2024 22:16 — with GitHub Actions Error

YixingZhang007 force-pushed the update_get_coordinate_tests branch from f613e96 to fc048c9 Compare October 30, 2024 22:18

YixingZhang007 had a problem deploying to WindowsCILock October 30, 2024 22:19 — with GitHub Actions Error

YixingZhang007 temporarily deployed to WindowsCILock October 30, 2024 22:20 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 30, 2024 22:34 — with GitHub Actions Inactive

YixingZhang007 force-pushed the update_get_coordinate_tests branch from b034c02 to be1187a Compare October 31, 2024 20:05

YixingZhang007 temporarily deployed to WindowsCILock October 31, 2024 20:20 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock October 31, 2024 21:52 — with GitHub Actions Inactive

YixingZhang007 had a problem deploying to WindowsCILock November 1, 2024 15:49 — with GitHub Actions Error

update XFAIL

fc18698

YixingZhang007 force-pushed the update_get_coordinate_tests branch from 2f361b6 to fc18698 Compare December 17, 2024 16:37

YixingZhang007 temporarily deployed to WindowsCILock December 17, 2024 16:37 — with GitHub Actions Inactive

YixingZhang007 had a problem deploying to WindowsCILock December 17, 2024 17:06 — with GitHub Actions Error

code clean up

e364d7e

YixingZhang007 temporarily deployed to WindowsCILock December 17, 2024 17:27 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock December 17, 2024 18:04 — with GitHub Actions Inactive

remove test

a299621

YixingZhang007 had a problem deploying to WindowsCILock December 18, 2024 15:27 — with GitHub Actions Error

test clean up

96d0916

YixingZhang007 had a problem deploying to WindowsCILock December 18, 2024 15:31 — with GitHub Actions Error

update number of no-supported test

d949187

YixingZhang007 temporarily deployed to WindowsCILock December 18, 2024 15:44 — with GitHub Actions Inactive

YixingZhang007 temporarily deployed to WindowsCILock December 18, 2024 16:14 — with GitHub Actions Inactive

YixingZhang007 marked this pull request as ready for review December 18, 2024 19:23

YixingZhang007 requested a review from a team as a code owner December 18, 2024 19:23

YixingZhang007 requested a review from againull December 18, 2024 19:23

dkhaldi reviewed Dec 18, 2024

View reviewed changes

remove Use != use::b

7115bf1

YixingZhang007 temporarily deployed to WindowsCILock December 18, 2024 21:08 — with GitHub Actions Inactive

dkhaldi approved these changes Dec 18, 2024

View reviewed changes

YixingZhang007 temporarily deployed to WindowsCILock December 18, 2024 21:52 — with GitHub Actions Inactive

againull approved these changes Dec 19, 2024

View reviewed changes

againull merged commit 7f465fc into intel:sycl Dec 19, 2024
14 checks passed

YuriPlyakhin reviewed Dec 19, 2024

View reviewed changes

YixingZhang007 mentioned this pull request Dec 19, 2024

[SYCL][E2E] Resolve reqd_sub_group_size issue for get_coordinate tests #16429

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SYCL][E2E] Update get_coordinate tests to verify both row and columns #15880

[SYCL][E2E] Update get_coordinate tests to verify both row and columns #15880

YixingZhang007 commented Oct 26, 2024 •

edited

Loading

YixingZhang007 commented Oct 26, 2024

uditagarwal97 commented Oct 26, 2024

dkhaldi Dec 18, 2024

dkhaldi left a comment

dkhaldi commented Dec 19, 2024

YixingZhang007 commented Dec 19, 2024

YuriPlyakhin Dec 19, 2024

YixingZhang007 Dec 19, 2024

YuriPlyakhin Dec 19, 2024

YixingZhang007 Dec 19, 2024

YuriPlyakhin Dec 19, 2024

YixingZhang007 Dec 19, 2024

[SYCL][E2E] Update get_coordinate tests to verify both row and columns #15880

[SYCL][E2E] Update get_coordinate tests to verify both row and columns #15880

Conversation

YixingZhang007 commented Oct 26, 2024 • edited Loading

YixingZhang007 commented Oct 26, 2024

uditagarwal97 commented Oct 26, 2024

dkhaldi Dec 18, 2024

Choose a reason for hiding this comment

dkhaldi left a comment

Choose a reason for hiding this comment

dkhaldi commented Dec 19, 2024

YixingZhang007 commented Dec 19, 2024

YuriPlyakhin Dec 19, 2024

Choose a reason for hiding this comment

YixingZhang007 Dec 19, 2024

Choose a reason for hiding this comment

YuriPlyakhin Dec 19, 2024

Choose a reason for hiding this comment

YixingZhang007 Dec 19, 2024

Choose a reason for hiding this comment

YuriPlyakhin Dec 19, 2024

Choose a reason for hiding this comment

YixingZhang007 Dec 19, 2024

Choose a reason for hiding this comment

YixingZhang007 commented Oct 26, 2024 •

edited

Loading