[SYCL][SPIR-V] Drop Unnecessary Matrix Use parameter #6923

MrSidims · 2022-09-30T13:59:05Z

Last parameter 'Use' is optional in SPIR-V, but is not optional in DPCPP headers. If it has Unnecessary value - skip it

Signed-off-by: Sidorov, Dmitry dmitry.sidorov@intel.com

Last parameter 'Use' is optional in SPIR-V, but is not optional in DPCPP headers. If it has Unnecessary value - skip it Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

jcranmer-intel · 2022-09-30T14:59:31Z

clang/lib/CodeGen/CodeGenTypes.cpp

@@ -57,11 +57,14 @@ void CodeGenTypes::addRecordTypeName(const RecordDecl *RD,
      if (auto TemplateDecl = dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
        ArrayRef<TemplateArgument> TemplateArgs =
            TemplateDecl->getTemplateArgs().asArray();
+        constexpr size_t MaxMatrixParameter = 6;
+        assert(TemplateArgs.size() <= MaxMatrixParameter &&
+               "Too many template parameters for JointMatrixINTEL type");


Is there a minimum matrix parameter we can also check at the same time?

Added, thanks!

Assert message should be updated right? Since you added a minimum requirement as well

Well, actually, after some consideration I realized, that there is no max or min number of template parameters, it's just a fixed number of 6.

Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

elizabethandrews · 2022-09-30T19:20:32Z

clang/lib/CodeGen/CodeGenTypes.cpp

@@ -57,11 +57,14 @@ void CodeGenTypes::addRecordTypeName(const RecordDecl *RD,
      if (auto TemplateDecl = dyn_cast<ClassTemplateSpecializationDecl>(RD)) {
        ArrayRef<TemplateArgument> TemplateArgs =
            TemplateDecl->getTemplateArgs().asArray();
+        constexpr size_t MaxMatrixParameter = 6;
+        assert(TemplateArgs.size() <= MaxMatrixParameter &&
+               "Too many template parameters for JointMatrixINTEL type");


Assert message should be updated right? Since you added a minimum requirement as well

elizabethandrews · 2022-09-30T19:24:07Z

clang/lib/CodeGen/CodeGenTypes.cpp

+            const auto IntTemplateParam = TemplateArgs[I].getAsIntegral();
+            // Last parameter 'Use' is optional in SPIR-V, but is not optional
+            // in DPCPP headers. If it has Unnecessary value - skip it
+            constexpr size_t Unnecessary = 3;


For my understanding can you explain this? What is the significance of '3' here?

What is the significance of '3' here?

comment would be helpful. Thank you

Added the comment

I'm still a little confused. My question was why is "3" = Unecessary. Is this defined by the spec? What are possible legal values for Use parameter?

No, there is no Unnecessary in both SPIR-V and SYCL spec. Legal values on user-visible API level are 0, 1 and 2 (same for SPIR-V). Yet Unnecessary is defined in intermediate level of SYCL headers to keep compatibility with previous version of the extension, where we didn't have Use parameter making to be a default Use value.

While answering your question another idea came up in my mind, which might be cleaner. What if we declare struct __spirv_JointMatrixINTEL depending on SYCL_EXT_ONEAPI_MATRIX , WDYT @yubingex007-a11y @dkhaldi ? Like this:

#if SYCL_EXT_ONEAPI_MATRIX == 1 template <typename T, std::size_t R, std::size_t C, MatrixLayout L, Scope::Flag S = Scope::Flag::Subgroup> struct __spirv_JointMatrixINTEL { T(*Value) [R][C][static_cast<size_t>(L) + 1][static_cast<size_t>(S) + 1]; }; #else template <typename T, std::size_t R, std::size_t C, MatrixLayout L, Scope::Flag S = Scope::Flag::Subgroup, MatrixUse U = MatrixUse::Unnecessary> struct __spirv_JointMatrixINTEL { T(*Value) [R][C][static_cast<size_t>(L) + 1][static_cast<size_t>(S) + 1] [static_cast<size_t>(U) + 1]; }; #endif // SYCL_EXT_ONEAPI_MATRIX

I like this idea as long this does not break current SPIRV (use is still optional).

Eh, there is quite a lot of things to change in the headers with a lot of code duplication to make it working. @elizabethandrews are we good to have the patch in clang or additional code commentary is required?
Basically every code which contains __spirv_JointMatrixINTEL should be duplicated and it includes all SPIR-V matrix instructions (10 of them) declarations with their appropriate calls in high-level SYCL API. So it does look for me that this clang modification is easier to support then two copies of the matrix-related code in the headers.

Can we do something like:

template <typename T, std::size_t R, std::size_t C, MatrixLayout L, Scope::Flag S = Scope::Flag::Subgroup #if SYCL_EXT_ONEAPI_MATRIX == 1 ,MatrixUse U = MatrixUse::Unnecessary> #else > #endif struct __spirv_JointMatrixINTEL { T(*Value) [R][C][static_cast<size_t>(L) + 1][static_cast<size_t>(S) + 1] #if SYCL_EXT_ONEAPI_MATRIX == 1 [static_cast<size_t>(U) + 1]; #else ; #endif // SYCL_EXT_ONEAPI_MATRIX };

Also, I see that Unnecessary template parameter is a default value, why can't we just declare it? How it will be incompatible with the previous version of the extension?

premanandrao

LGTM other than the couple of things that @elizabethandrews pointed out already.

Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

MrSidims · 2022-10-04T11:48:05Z

Converted to draft.
Will close in favor of #6957 if internal testing will finish OK.

MrSidims requested a review from a team as a code owner September 30, 2022 13:59

MrSidims requested review from dkhaldi and jcranmer-intel September 30, 2022 13:59

[SYCL][SPIR-V] Drop Unnecessary Matrix Use parameter

3a136cb

Last parameter 'Use' is optional in SPIR-V, but is not optional in DPCPP headers. If it has Unnecessary value - skip it Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

MrSidims force-pushed the private/MrSidims/FixMatrixUseTypename branch from edd0de4 to 3a136cb Compare September 30, 2022 14:57

jcranmer-intel reviewed Sep 30, 2022

View reviewed changes

Add min number of template parameters check

1405c9d

Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

MrSidims force-pushed the private/MrSidims/FixMatrixUseTypename branch from e95bf92 to 1405c9d Compare September 30, 2022 15:12

MrSidims requested a review from jcranmer-intel September 30, 2022 15:15

elizabethandrews reviewed Sep 30, 2022

View reviewed changes

premanandrao reviewed Sep 30, 2022

View reviewed changes

Remove min template params

141524c

Signed-off-by: Sidorov, Dmitry <dmitry.sidorov@intel.com>

MrSidims marked this pull request as draft October 4, 2022 11:47

MrSidims closed this Oct 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SYCL][SPIR-V] Drop Unnecessary Matrix Use parameter #6923

[SYCL][SPIR-V] Drop Unnecessary Matrix Use parameter #6923

Uh oh!

MrSidims commented Sep 30, 2022

Uh oh!

jcranmer-intel Sep 30, 2022

Uh oh!

MrSidims Sep 30, 2022

Uh oh!

elizabethandrews Sep 30, 2022

Uh oh!

MrSidims Oct 3, 2022

Uh oh!

elizabethandrews Sep 30, 2022

Uh oh!

elizabethandrews Sep 30, 2022

Uh oh!

smanna12 Sep 30, 2022

Uh oh!

MrSidims Oct 3, 2022

Uh oh!

elizabethandrews Oct 3, 2022

Uh oh!

MrSidims Oct 3, 2022

Uh oh!

dkhaldi Oct 3, 2022

Uh oh!

MrSidims Oct 3, 2022 •

edited

Loading

Uh oh!

Fznamznon Oct 4, 2022

Uh oh!

premanandrao left a comment

Uh oh!

MrSidims commented Oct 4, 2022 •

edited

Loading

Uh oh!

Uh oh!

[SYCL][SPIR-V] Drop Unnecessary Matrix Use parameter #6923

[SYCL][SPIR-V] Drop Unnecessary Matrix Use parameter #6923

Uh oh!

Conversation

MrSidims commented Sep 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrSidims Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

premanandrao left a comment

Choose a reason for hiding this comment

Uh oh!

MrSidims commented Oct 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

MrSidims Oct 3, 2022 •

edited

Loading

MrSidims commented Oct 4, 2022 •

edited

Loading