[SYCL][Matrix] test the two features: fill a matrix and element wise operations by dkhaldi · Pull Request #645 · intel/llvm-test-suite

dkhaldi · 2021-12-20T15:58:22Z

Two new matrix features are being added to the DPC++ compiler namely: fill a matrix and element wise operations. This PR adds tests for these two features.

Signed-off-by: Dounia Khaldi dounia.khaldi@intel.com

vladimirlaz · 2021-12-21T05:33:58Z

*vector_relational test failure is unrelated and can be ignored.

…operations Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

AlexeySotkin · 2021-12-23T08:18:36Z

SYCL/Matrix/element_wise_ops.cpp

+
+  big_matrix<int32_t, MATRIX_M, MATRIX_N> MC((int32_t *)&C);
+  big_matrix<int32_t, MATRIX_M, MATRIX_N> MD((int32_t *)&D);
+  big_matrix<int8_t, MATRIX_M, MATRIX_K> MA((int8_t *)&A);


I think we should have a test with other types too, with sycl::half for example.

AlexeySotkin · 2021-12-23T08:31:48Z

SYCL/Matrix/element_wise_ops.cpp

+           }
+           auto wi_slice_c = sub_c.get_wi_data();
+           for (int i = 0; i < wi_slice_c.length(); i++) {
+             wi_slice_c[i] *= 2;


Other operations (+=, /=, <<=, %=, .. see the full list here) and syntax (wi_slice_c[i] = wi_slice_c[i] * 2) should be covered.

…l other operations Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

…ches are not merged yet Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

cperkinsintel · 2022-01-06T23:44:35Z

@dkhaldi is there a matching PR on intel/llvm that implements these new matrix features?

cperkinsintel · 2022-01-06T23:52:50Z

SYCL/Matrix/joint_matrix_ss_int8.cpp

 // RUN: %GPU_RUN_PLACEHOLDER %t.out

+// XFAIL: *
+


If this is just always going to fail, is there any plan to fix or support it? If so, it would be good to mention it in a comment, otherwise someone may just remove this test.

I added xfail because the test will fail until two internal patches that are currently under review get merged.
Once all the patches are in, I will remove xfail.
I just double checked with @yuxianch, the test should be marked as xfail.

But I will add a comment as you suggested

dkhaldi · 2022-01-07T01:56:00Z

@dkhaldi is there a matching PR on intel/llvm that implements these new matrix features?

These two features have been implemented. See intel/llvm#4979 for the slicing and intel/llvm#4994 for fill.
However, there are two more patches coming to add the missing operators and one bug fix to be composed by @yubingex007-a11y .

dkhaldi · 2022-01-07T01:58:44Z

@cperkinsintel is SYCL :: KernelAndProgram/multiple-kernel-linking.cpp a known fail test?

yubingex007-a11y

LGTM

dkhaldi · 2022-01-07T14:49:21Z

@intel/llvm-reviewers-runtime ping to merge as this needs to be cherrypicked to the internal repo as well

cperkinsintel

Thanks for the links to the PRs that introduce the new features. This LGTM.

yubingex007-a11y · 2022-01-11T08:15:47Z

SYCL/Matrix/element_wise_all_ops_half.cpp

+         if (wi_slice_a[i]) {
+           if (wi_slice_a[i] > 2.0 || wi_slice_a[i] >= 2.0 ||
+               wi_slice_a[i] < 2.0 || wi_slice_a[i] <= 2.0) {
+             T val = (wi_slice_a[i] != 2.0) ? wi_slice_a[i] : 2.0;


I think we should change the testcase according to the rules of "?: " and use static_cast for ternary operator in our code.

If the types of the second and third operands are not identical, then complex type conversion rules, as specified in the C++ Standard, are invoked. These conversions may lead to unexpected behavior including construction and destruction of temporary objects. For this reason, we strongly advise you to either (1) avoid using user-defined types as operands with the conditional operator or (2) if you do use user-defined types, then explicitly cast each operand to a common type.

yubingex007-a11y · 2022-01-11T08:19:35Z

SYCL/Matrix/element_wise_all_ops_half.cpp

+         if (wi_slice_a[i]) {
+           if (wi_slice_a[i] > 2.0 || wi_slice_a[i] >= 2.0 ||
+               wi_slice_a[i] < 2.0 || wi_slice_a[i] <= 2.0) {
+             T val = (wi_slice_a[i] != 2.0) ? wi_slice_a[i] : 2.0;


Suggested change

T val = (wi_slice_a[i] != 2.0) ? wi_slice_a[i] : 2.0;

T val = (wi_slice_a[i] != 2.0) ? wi_slice_a[i] : static_cast<half>(2.0);

@yubingex007-a11y thank you for fixing the two bugs related to fill and slicing when type deduction confusion happens, I made this change in a separate PR #727.

…operations (intel#645) Two new matrix features are being added to the DPC++ compiler namely: fill a matrix and element wise operations. This PR adds tests for these two features. Signed-off-by: Dounia Khaldi dounia.khaldi@intel.com

…operations (intel/llvm-test-suite#645) Two new matrix features are being added to the DPC++ compiler namely: fill a matrix and element wise operations. This PR adds tests for these two features. Signed-off-by: Dounia Khaldi dounia.khaldi@intel.com

dkhaldi requested review from bader, romanovvlad and vladimirlaz as code owners December 20, 2021 15:58

dkhaldi requested a review from yubingex007-a11y December 20, 2021 15:59

vladimirlaz previously approved these changes Dec 21, 2021

View reviewed changes

dkhaldi added 2 commits December 21, 2021 10:36

[SYCL][Matrix] test the two features: fill a matrix and element wise …

4e144b1

…operations Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

[SYCL][Matrix][Element-wise-ops] correct the reference multiplication

2c66267

Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

dkhaldi dismissed vladimirlaz’s stale review via 2c66267 December 21, 2021 15:37

dkhaldi force-pushed the elem-wise-ops branch from b3723ec to 2c66267 Compare December 21, 2021 15:37

AlexeySotkin reviewed Dec 23, 2021

View reviewed changes

[SYCL][Matrix] Incorporate Alexey comments by adding half type and al…

51203d1

…l other operations Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

dkhaldi requested a review from a team as a code owner January 6, 2022 02:16

[SYCL][Matrix] Add xfail to the modified test because the backend pat…

7ca403f

…ches are not merged yet Signed-off-by: Dounia Khaldi <dounia.khaldi@intel.com>

dkhaldi requested a review from cperkinsintel January 6, 2022 20:55

cperkinsintel reviewed Jan 6, 2022

View reviewed changes

dkhaldi mentioned this pull request Jan 7, 2022

Flaky test: KernelAndProgram/multiple-kernel-linking.cpp #710

Closed

dkhaldi added 3 commits January 6, 2022 22:08

[SYCL][Matrix] Add a comment to explain why the XFAIL is temporary

6d126d1

[SYCL][Matrix] formatting

e5026c6

[SYCL][Matrix] Add XFAIL because of a known bug

6f812d1

yubingex007-a11y approved these changes Jan 7, 2022

View reviewed changes

cperkinsintel approved these changes Jan 7, 2022

View reviewed changes

cperkinsintel requested a review from AlexeySotkin January 7, 2022 18:14

pvchupin merged commit 6e868ff into intel:intel Jan 7, 2022

yubingex007-a11y reviewed Jan 11, 2022

View reviewed changes

	T val = (wi_slice_a[i] != 2.0) ? wi_slice_a[i] : 2.0;
	T val = (wi_slice_a[i] != 2.0) ? wi_slice_a[i] : static_cast<half>(2.0);

Comments

Conversation

dkhaldi commented Dec 20, 2021

Uh oh!

vladimirlaz commented Dec 21, 2021

Uh oh!

AlexeySotkin Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

AlexeySotkin Dec 23, 2021

Choose a reason for hiding this comment

Uh oh!

cperkinsintel commented Jan 6, 2022

Uh oh!

cperkinsintel Jan 6, 2022

Choose a reason for hiding this comment

Uh oh!

dkhaldi Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkhaldi commented Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkhaldi commented Jan 7, 2022

Uh oh!

yubingex007-a11y left a comment

Choose a reason for hiding this comment

Uh oh!

dkhaldi commented Jan 7, 2022

Uh oh!

cperkinsintel left a comment

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

dkhaldi Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dkhaldi Jan 7, 2022 •

edited

Loading

dkhaldi commented Jan 7, 2022 •

edited

Loading

yubingex007-a11y Jan 11, 2022 •

edited

Loading

dkhaldi Jan 11, 2022 •

edited

Loading