[SYCL][matrix] Update the query interface with the latest joint matrix approved syntax #11004

dkhaldi · 2023-08-29T15:28:15Z

No description provided.

…x approved syntax

dkhaldi · 2023-08-29T15:32:25Z

This PR replaces #10847. It addresses most of the comments raised there.
An outstanding issue is on how to add a new value for CPU device (intel_cpu_spr), see #10847 (comment)

dkhaldi · 2023-08-29T16:21:20Z

It's more complicated than adding new item here for CPU device: it won't be usable if user tries to use it in device AOT or host scenarios.
(1) What we can do right now is that we can add explicit limitation to the spec and add the actual functionality later
(2) Another option is to introduce all intel_cpu_* items as separate PR and make this PR dependent on that PR (e.g., regarding host scenario we already have the updated OpenCL extension in OpenCL CPU RT which allows us to query the unique IDs of Intel CPU architectures)
@gmlueck what do you think between (1) and (2) or maybe other option?

I'd like to have a plan for how this new SPR enumerator fits in with all of the other uses of the architecture enum. Currently, all of the following are related:

The enumerators in architecture.
The legal value for the -fsycl-targets command line switch.
The if_architecture_is function that can be called from device code.
The device::ext_oneapi_architecture_is function that can be called from host code.
It seems like it would be nice to support syntax like -fsycl-targets=intel_cpu_spr, which would AOT compile device code for SPR. This would provide a consistent way to AOT compile for either GPU or CPU devices. I think this would also enable support for if_architecture_is as a side-effect.

For device::ext_oneapi_architecture_is, can we use the CPUID instruction to determine if the CPU is SPR?

It might be useful to have a meeting to discuss this further.

Currently -march=sapphirerapids can be used to specify compilation for SPR.
@yubingex007-a11y, is there something like -fsycl-targets=intel_cpu_spr for SPR?
Yes, I have used CPUID to detect whether this is SPR but not sure this is reliable:
(model == 15 && family == 6 && extended_model == 8 &&
extended_family == 0)

sycl/include/sycl/ext/oneapi/matrix/static-query-use.hpp

sycl/doc/extensions/experimental/sycl_ext_matrix/sycl_ext_oneapi_matrix.asciidoc

steffenlarsen

LGTM!

dkhaldi · 2023-09-11T18:57:58Z

In an offline discussion, we agreed on adding the new SPR enumerator intel_cpu_spr to the list of devices.
But we will need to confirm that there are no issues/objections on having -fsycl-targets include intel_cpu_spr for AOT compilation.

…x_type::sint32

…ix::combinations to matrix_combinations

dkhaldi · 2023-09-15T16:05:16Z

@dm-vodopyanov, @gmlueck can you please approve the spec changes?

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc

YuriPlyakhin

LGTM

… names, dg2 is used

dm-vodopyanov · 2023-09-19T12:02:49Z

SYCL Pre Commit on Windows / Windows / SYCL E2E on Windows (pull_request) https://github.com/intel/llvm/actions/runs/6223657584/job/16898974221?pr=11004

seems like a common CI problem.

@dkhaldi is this patch ready to be merged?

dm-vodopyanov · 2023-09-19T12:18:49Z

@dkhaldi oh last-minute comment: spec has usage of matrix_type::int8, but there is no int8 in matrix_type enum. Should it be sint8? I can fix it together with my upcoming patch.

dkhaldi · 2023-09-19T16:45:25Z

@dkhaldi oh last-minute comment: spec has usage of matrix_type::int8, but there is no int8 in matrix_type enum. Should it be sint8? I can fix it together with my upcoming patch.

Thanks, I fixed it here.

dkhaldi · 2023-09-19T16:57:46Z

@gmlueck, do the recent changes to the SPR addition look good now? If so, please approve the spec portion.

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc

dm-vodopyanov · 2023-09-21T08:10:48Z

sycl/test/matrix/runtime-query.cpp

+            << combinations.size() << std::endl;
+
+  bool max_sizes;
+  if (combinations[0].maxsize == 0)


What is maxsize? There is no maxsize in combination struct. I will fix in the upcoming patch and enable (and re-write) the test for PVC, as this is a platform where we definitely test in this particular CI.

dm-vodopyanov · 2023-09-21T12:19:11Z

sycl/test/matrix/runtime-query.cpp

+void matrix_runtime_query(queue q) {
+
+  std::vector<combination> combinations =
+      q.get_device().get_info<sycl::info::device::matrix_combinations>();


sycl::ext::oneapi::experimental::info::device::matrix_combinations

yubingex007-a11y · 2023-09-21T12:26:30Z

sycl/test-e2e/Matrix/joint_matrix_query_default.cpp

@@ -39,7 +39,8 @@ void matrix_multiply(big_matrix<T1, NUM_ROWS_C, NUM_COLS_C> &C,
  size_t K = NUM_COLS_A;
  assert(NUM_ROWS_C == NUM_ROWS_A && NUM_COLS_A == NUM_ROWS_B * 4);

-  using myparams2 = tpu_params<tpu::amx, int8_t, int8_t, int>;
+  using myparams2 =
+      matrix_params<architecture::intel_cpu_spr, int8_t, int8_t, int>;


should be sycl::ext::oneapi::experimental::architecture.
otherwise, errmsg is reported
error: use of undeclared identifier 'architecture'

[SYCL][matrix] Update the query interface with the latest joint matri…

fc98c22

…x approved syntax

dkhaldi requested review from YuriPlyakhin, yubingex007-a11y and a team as code owners August 29, 2023 15:28

dkhaldi requested a review from steffenlarsen August 29, 2023 15:28

dkhaldi mentioned this pull request Aug 29, 2023

[SYCL][matrix] Update the query interface with the latest joint matrix approved syntax #10847

Closed

YuriPlyakhin reviewed Aug 29, 2023

View reviewed changes

sycl/include/sycl/ext/oneapi/matrix/static-query-use.hpp Outdated Show resolved Hide resolved

YuriPlyakhin reviewed Aug 29, 2023

View reviewed changes

sycl/include/sycl/ext/oneapi/matrix/static-query-use.hpp Outdated Show resolved Hide resolved

YuriPlyakhin reviewed Aug 29, 2023

View reviewed changes

sycl/include/sycl/ext/oneapi/matrix/static-query-use.hpp Outdated Show resolved Hide resolved

YuriPlyakhin reviewed Aug 29, 2023

View reviewed changes

sycl/include/sycl/ext/oneapi/matrix/static-query-use.hpp Outdated Show resolved Hide resolved

Add dg2 names

3c42cac

gmlueck reviewed Sep 6, 2023

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_matrix/sycl_ext_oneapi_matrix.asciidoc Show resolved Hide resolved

steffenlarsen approved these changes Sep 7, 2023

View reviewed changes

Address Greg's comment: merge blank cells

3ca9bfd

dkhaldi added 4 commits September 14, 2023 09:02

fix typo in XMX and NVIDIA tables: matrix_type::int32 should be matri…

833b8da

…x_type::sint32

Add clarification about SPR enumerator AOT limitation and rename matr…

afe6471

…ix::combinations to matrix_combinations

minor: change test name

1092032

minor: fix directory accident change

ca43df6

dkhaldi added 2 commits September 15, 2023 09:23

fix format

7164c66

test name change again to avoid format issue

06b2deb

gmlueck reviewed Sep 15, 2023

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc Outdated Show resolved Hide resolved

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc Outdated Show resolved Hide resolved

YuriPlyakhin approved these changes Sep 15, 2023

View reviewed changes

dkhaldi added 2 commits September 18, 2023 07:00

Address Greg's comments and remove ATS from the test case because for…

7afef97

… names, dg2 is used

fix formatting

430b666

Address Dmitry comment

0767357

gmlueck reviewed Sep 20, 2023

View reviewed changes

sycl/doc/extensions/experimental/sycl_ext_oneapi_device_architecture.asciidoc Show resolved Hide resolved

Add blank line for SPR limitation paragraph

81ace90

gmlueck approved these changes Sep 20, 2023

View reviewed changes

dkhaldi temporarily deployed to WindowsCILock September 20, 2023 19:19 — with GitHub Actions Inactive

dkhaldi temporarily deployed to WindowsCILock September 20, 2023 20:16 — with GitHub Actions Inactive

dm-vodopyanov reviewed Sep 21, 2023

View reviewed changes

dm-vodopyanov merged commit 56de25d into intel:sycl Sep 21, 2023

dm-vodopyanov reviewed Sep 21, 2023

View reviewed changes

yubingex007-a11y reviewed Sep 21, 2023

View reviewed changes

dkhaldi deleted the query-update3 branch September 25, 2023 17:01

JackAKirk mentioned this pull request Sep 29, 2023

"Native API returns: -42 (PI_ERROR_INVALID_BINARY)" when running any example from joint_matrix extension #11362

Closed

[SYCL][matrix] Update the query interface with the latest joint matrix approved syntax #11004

[SYCL][matrix] Update the query interface with the latest joint matrix approved syntax #11004

Uh oh!

Conversation

dkhaldi commented Aug 29, 2023

Uh oh!

dkhaldi commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dkhaldi commented Aug 29, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

steffenlarsen left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

dkhaldi commented Sep 15, 2023

Uh oh!

Uh oh!

Uh oh!

YuriPlyakhin left a comment

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov commented Sep 19, 2023

Uh oh!

dm-vodopyanov commented Sep 19, 2023

Uh oh!

dkhaldi commented Sep 19, 2023

Uh oh!

dkhaldi commented Sep 19, 2023

Uh oh!

Uh oh!

dm-vodopyanov Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

dm-vodopyanov Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

yubingex007-a11y Sep 21, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dkhaldi commented Aug 29, 2023 •

edited

Loading

dkhaldi commented Sep 11, 2023 •

edited

Loading