[Matrix] Enable joint_matrix_fill for joint_matrix feature by yubingex007-a11y · Pull Request #4994 · intel/llvm

yubingex007-a11y · 2021-11-19T16:12:09Z

No description provided.

dkhaldi · 2021-11-19T16:20:48Z

sycl/test/matrix/matrix-int8-test-fill.cpp

+
+           // AMX: 8 register tiles : 1k byte size, SMmaxxSKmax =16x64
+           // strideX = X's cols, so strideC = N, strideA = K, strideB = N*4
+           joint_matrix_fill(sg, sub_c, 0);


Instead of adding a new test, I would suggest changing the existing one to use fill instead of load for C.

dkhaldi · 2021-11-19T16:21:07Z

sycl/test/matrix/matrix-int8-test-fill.cpp

+           // AMX: 8 register tiles : 1k byte size, SMmaxxSKmax =16x64
+           // strideX = X's cols, so strideC = N, strideA = K, strideB = N*4
+           joint_matrix_fill(sg, sub_c, 0);
+           joint_matrix_load(sg, sub_c,


load for C is redundant here

MrSidims · 2021-11-25T10:46:04Z

sycl/include/CL/__spirv/spirv_ops.hpp

+          __spv::MatrixLayout L = __spv::MatrixLayout::RowMajor,
+          __spv::Scope::Flag S = __spv::Scope::Flag::Subgroup>
+extern SYCL_EXTERNAL __spv::__spirv_JointMatrixINTEL<T, R, C, L, S> *
+__spirv_JointMatrixFillINTEL(const T &v, __spv::Scope::Flag Sc = S);


Suggested change

__spirv_JointMatrixFillINTEL(const T &v, __spv::Scope::Flag Sc = S);

__spirv_CompositeConstruct(const T &v);

Note, there is not Scope flag for the instruction, but it should be OK, since it presence in the result type.

Suggested change

__spirv_JointMatrixFillINTEL(const T &v, __spv::Scope::Flag Sc = S);

__spirv_JointMatrixFillINTEL(const T v, __spv::Scope::Flag Sc = S);

Like const T& the compiler will emit a temporary reference of value T, while we want to pass a constant expression to the instructions below and not load from this temporary reference.

yubingex007-a11y · 2021-12-14T14:04:38Z

Ping?

MrSidims · 2021-12-14T15:16:47Z

sycl/include/sycl/ext/oneapi/matrix/matrix-jit.hpp

+joint_matrix_fill(Group sg,
+                  joint_matrix<T, NumRows, NumCols, Layout, Group> &res,
+                  const T v) {


Suggested change

joint_matrix_fill(Group sg,

joint_matrix<T, NumRows, NumCols, Layout, Group> &res,

const T v) {

joint_matrix_fill(joint_matrix<T, NumRows, NumCols, Layout, Group> &res,

const T v) {

sg was unused

Since we are not using scope on the SPIRV instruction as we are using the existing spirv_CompositeConstruct instruction. This argument will remain unused.
But we still need it on the DPC++ function to match the other DPC++ functions

In this case please cast it to void, otherwise the compiler will emit a diagnostic.

MrSidims

Other than unused scope the patch is LGTM.

dkhaldi

LGTM

yubingex007-a11y · 2021-12-15T06:48:44Z

ping again.

MrSidims · 2021-12-15T08:35:10Z

sycl/include/sycl/ext/oneapi/matrix/matrix-jit.hpp

+joint_matrix_fill(Group sg,
+                  joint_matrix<T, NumRows, NumCols, Layout, Group> &res,
+                  const T v) {
+  (void)sg;


nit: might be worth to put a comment here, why we kept (sg) parameter (to be aligned with other AMX API?, to may be replace __spirv_CompositeConstruct with some other instruction in case if we are not satisficed without breaking API/ABI with returning 'sg' back?)

dkhaldi

LGTM

yubingex007-a11y · 2021-12-21T16:02:10Z

ping? @intel/llvm-reviewers-runtime

yubingex007-a11y · 2021-12-22T07:56:08Z

@bader is the fail in pre-checkin is known issue?

bader · 2021-12-22T09:41:12Z

@bader is the fail in pre-checkin is known issue?

I haven't seen DiscardEvents/invalid_event.cpp failures before, but it's better to ask test owners.

dkhaldi

LGTM

yubingex007-a11y · 2021-12-23T06:47:15Z

@intel/llvm-reviewers-runtime ping?
The basic_event_collection.cpp is expected I think. I saw intel/llvm-test-suite#658 disable this case.

yubingex007-a11y · 2021-12-23T06:47:49Z

@bader ping?
The basic_event_collection.cpp is expected I think. I saw intel/llvm-test-suite#658 disable this case.

bader · 2021-12-23T07:47:32Z

@bader ping?

Waiting on code owner review from intel/llvm-reviewers-runtime.

The basic_event_collection.cpp is expected I think. I saw intel/llvm-test-suite#658 disable this case.

The test was re-enabled by intel/llvm-test-suite#659

yubingex007-a11y · 2021-12-23T14:25:34Z

@intel/llvm-reviewers-runtime ping?

yubingex007-a11y · 2021-12-24T05:23:21Z

@vladimirlaz ping?

yubingex007-a11y · 2021-12-24T07:17:34Z

The failed test in SYCL / Default Linux / HIP AMD GPU Test Suite can also be observed in other PRs such as #5218

yubingex007-a11y · 2021-12-24T07:17:50Z

@bader ping

vladimirlaz · 2021-12-24T07:41:07Z

@AGindinson is working on free_function_* failure @psamolysov-intel is working on AtomicRef failures. The ticket is ready for merge

bader · 2021-12-24T08:11:37Z

The ticket is ready for merge

@vladimirlaz, please, approve and merge.

[Matrix] Enable joint_matrix_fill for joint_matrix feature

ef015f4

yubingex007-a11y requested a review from a team as a code owner November 19, 2021 16:12

yubingex007-a11y requested review from AlexeySotkin, LuoYuanke, MrSidims, dkhaldi and sergey-semenov November 19, 2021 16:12

dkhaldi reviewed Nov 19, 2021

View reviewed changes

MrSidims reviewed Nov 25, 2021

View reviewed changes

yubingex007-a11y force-pushed the fill branch from 339a0eb to d1fc3c6 Compare December 9, 2021 15:36

Address Douniai&Dmitry's comments

978ec48

yubingex007-a11y force-pushed the fill branch from d1fc3c6 to 978ec48 Compare December 9, 2021 15:44

yubingex007-a11y requested review from MrSidims and dkhaldi December 14, 2021 14:04

MrSidims reviewed Dec 14, 2021

View reviewed changes

dkhaldi previously approved these changes Dec 14, 2021

View reviewed changes

Set XFAIL to testcase and solve unused param's issue

6f30655

yubingex007-a11y dismissed dkhaldi’s stale review via 6f30655 December 15, 2021 06:42

MrSidims previously approved these changes Dec 15, 2021

View reviewed changes

MrSidims reviewed Dec 15, 2021

View reviewed changes

adding comments for unused sg

35e59fb

yubingex007-a11y dismissed MrSidims’s stale review via 35e59fb December 21, 2021 15:59

dkhaldi previously approved these changes Dec 21, 2021

View reviewed changes

vladimirlaz previously approved these changes Dec 21, 2021

View reviewed changes

bader mentioned this pull request Dec 22, 2021

[SYCL] Disable Basic/get_backend test intel/llvm-test-suite#649

Merged

Merge remote-tracking branch 'intel_llvm/sycl' into fill

e07b931

yubingex007-a11y dismissed stale reviews from vladimirlaz and dkhaldi via e07b931 December 22, 2021 15:03

yubingex007-a11y requested review from MrSidims, dkhaldi and vladimirlaz and removed request for MrSidims December 22, 2021 15:04

dkhaldi approved these changes Dec 22, 2021

View reviewed changes

vladimirlaz approved these changes Dec 24, 2021

View reviewed changes

vladimirlaz merged commit cbad428 into intel:sycl Dec 24, 2021

dkhaldi mentioned this pull request Jan 7, 2022

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

Merged

	__spirv_JointMatrixFillINTEL(const T &v, __spv::Scope::Flag Sc = S);
	__spirv_CompositeConstruct(const T &v);

	__spirv_JointMatrixFillINTEL(const T &v, __spv::Scope::Flag Sc = S);
	__spirv_JointMatrixFillINTEL(const T v, __spv::Scope::Flag Sc = S);

Comments

Conversation

yubingex007-a11y commented Nov 19, 2021

Uh oh!

dkhaldi Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

dkhaldi Nov 19, 2021

Choose a reason for hiding this comment

Uh oh!

MrSidims Nov 25, 2021

Choose a reason for hiding this comment

Uh oh!

MrSidims Dec 6, 2021

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y commented Dec 14, 2021

Uh oh!

MrSidims Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

dkhaldi Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

MrSidims Dec 14, 2021

Choose a reason for hiding this comment

Uh oh!

MrSidims left a comment

Choose a reason for hiding this comment

Uh oh!

dkhaldi left a comment

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y commented Dec 15, 2021

Uh oh!

MrSidims Dec 15, 2021 • 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 left a comment

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y commented Dec 21, 2021

Uh oh!

yubingex007-a11y commented Dec 22, 2021

Uh oh!

bader commented Dec 22, 2021

Uh oh!

dkhaldi left a comment

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y commented Dec 23, 2021

Uh oh!

yubingex007-a11y commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bader commented Dec 23, 2021

Uh oh!

yubingex007-a11y commented Dec 23, 2021

Uh oh!

yubingex007-a11y commented Dec 24, 2021

Uh oh!

yubingex007-a11y commented Dec 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yubingex007-a11y commented Dec 24, 2021

Uh oh!

vladimirlaz commented Dec 24, 2021

Uh oh!

bader commented Dec 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MrSidims Dec 15, 2021 •

edited

Loading

yubingex007-a11y commented Dec 23, 2021 •

edited

Loading

yubingex007-a11y commented Dec 24, 2021 •

edited

Loading