[SPIR-V][DOC] Add 'Use' parameter to TypeJointMatrixINTEL #5944

MrSidims · 2022-04-01T11:45:15Z

'Use' is an optional parameter that shows where in a math operation the matrix
is used. It must be the result of a constant instruction with scalar
'integer type'.

Signed-off-by: Dmitry Sidorov [email protected]

'Use' is an optional parameter that shows where in a math operation the matrix is used. It must be the result of a constant instruction with scalar 'integer type'. Signed-off-by: Dmitry Sidorov <[email protected]>

'Use' is an optional parameter that shows where in a math operation the matrix is used. It must be the result of a constant instruction with scalar 'integer type'. Spec: intel/llvm#5944 Signed-off-by: Dmitry Sidorov <[email protected]>

'Use' is an optional parameter that shows where in a math operation the matrix is used. It must be the result of a constant instruction with scalar 'integer type'. Spec: intel#5944 Signed-off-by: Dmitry Sidorov <[email protected]> Original commit: KhronosGroup/SPIRV-LLVM-Translator@348da24

'Use' is an optional parameter that shows where in a math operation the matrix is used. It must be the result of a constant instruction with scalar 'integer type'. Spec: intel/llvm#5944 Signed-off-by: Dmitry Sidorov <[email protected]> (cherry picked from commit 348da24)

'Use' is an optional parameter that shows where in a math operation the matrix is used. It must be the result of a constant instruction with scalar 'integer type'. Spec: intel/llvm#5944 Signed-off-by: Dmitry Sidorov <[email protected]>

AlexeySachkov · 2022-08-10T19:56:05Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc



 1+|Capability: +
 *{capability_name}*
-1+| 7 | {OpTypeJointMatrixINTEL_token}
+1+| 8 | {OpTypeJointMatrixINTEL_token}


Use is an optional argument, which means that operands count is not always 8, it should probably say 7+ instead

Done, thanks!

AlexeySachkov · 2022-08-10T19:59:40Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

@@ -192,11 +200,15 @@ It must be the result of a constant instruction. +
 +
 'Scope' is memory scope for operations on the matrix. It must be the
 result of a constant instruction with scalar 'integer type'. +
+ +
+'Use' is an optional parameter that shows where in a math operation the matrix
+is used. It must be the result of a constant instruction with scalar


Integer type is already defined as scalar in the core spec, so you can simplify this definition and probably spell integer starting with uppercase letter to highlight that you are referring to an existing term.

Also: do we need to explicitly specify that the value is expected to be from Use table above and not just any arbitrary integer (like 42, for example)?

well, it makes sense, but "'integer type' scalar" is a commonly used wording across https://registry.khronos.org/SPIR-V/specs/unified1/SPIRV.pdf . Regarding the values - added this info, though there is some misalignments in the spec, guess, which should be corrected.

I think it would be best to document this similar to the way Scope <id>s are documented:

Must be an <id> of a 32-bit integer scalar. Its value is expected to be one of the values in the table below. If validation rules or the client API require a constant <id>, it is invalid for it to not be one of these values. If non-constant <id> are allowed, behavior is undefined if <id> is not one of these values.

Since the Use is an optional parameter what is the behavior if the optional parameter is omitted?

Thanks for the great suggestion, Ben! I've also aligned Layout parameter with this definition. And I'll adopt it for Scope as well.

Since the Use is an optional parameter what is the behavior if the optional parameter is omitted?

Per new revision it's now a mandatory parameter, before that it was up to the devices' backends to decide.

Signed-off-by: dmitry.sidorov <[email protected]>

…matrix-use

Signed-off-by: Sidorov, Dmitry <[email protected]>

bashbaug

Hi, I have a few questions and found a few spelling errors.

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

bashbaug · 2022-09-08T20:58:48Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

 'Layout' indicates how the values loaded from memory are arranged. 
-It must be the result of a constant instruction. +
- +
-'Scope' is syncronization scope for operation on the matrix. It must be the
-result of a constant instruction with scalar 'integer type'. +
+Must be an '<id>' of a 32-bit integer scalar. Its value is expected to be one of
+the values in the table 3.XX, Matrix Layout. If validation rules or the client API
+require a constant '<id>', it is invalid for it to not be one of these values.
+If non-constant '<id>' are allowed, behavior is undefined if '<id>' is not one of
+these values. +


Should this be changed to say that "Layout" indicates how the values in memory are arranged? Presumably the way the values loaded from memory are arranged are indicated by the "Layout" of the OpTypeJointMatrix type Result Type instead.

Also, just to confirm: there are no requirements that the two layouts match, and the full set of combinations must be supported?

Applied

Also, just to confirm: there are no requirements that the two layouts match, and the full set of combinations must be supported?

AFAIU yes

bashbaug · 2022-09-08T21:35:53Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

@@ -270,11 +306,19 @@ where elements of the matrix must be stored and arranged according to 'Layout'.
 'Stride' is the number of elements in memory between beginnings of successive
 rows, columns (or words) of the 'Object'. It must be a scalar integer type. +
 +
-'Layout' indicates how the values stored to memory are arranged. It must be the
-result of a constant instruction. +
+'Layout' indicates how the values stored to memory are arranged.


Similar to the load instruction, I would find this sentence to be clearer if it were changed to say that it indicates how the values that are stored are arranged in memory (assuming this is the intent).

bashbaug · 2022-09-08T21:40:10Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

+either _Workgroup_ or _Subgroup_ from the table 3.27. Scope <id>. If validation
+rules or the client API require a constant '<id>', it is invalid for it to not
+be one of these values. If non-constant '<id>' are allowed, behavior is
+undefined if '<id>' is not one of these values. +


I'm guessing there are some other requirements for the "Use" of the A, B, and C matrix operands that should be documented here? As an example it is probably an error (or, if it cannot be statically checked, behavior is undefined) if the "Use" for the A matrix is MatrixB or Accumulator?

Same goes for all other variants of OpJointMatrixXXMadINTEL.

bashbaug · 2022-09-08T21:42:35Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

+| 0 | *MatrixA*     | *{capability_name}*
+| 1 | *MatrixB*     | *{capability_name}*
+| 2 | *Accumulator* | *{capability_name}*
+| 3 | *Unnecessary* | *{capability_name}*


The Unnecessary use is never documented anywhere in this extension. Should it be?

That's actually a spec bug, there shouldn't be any Unnecessary value for Use parameter, instead there should be Unused Layout value. I'll update the spec

Signed-off-by: Sidorov, Dmitry <[email protected]>

dkhaldi · 2022-10-13T15:50:35Z

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

-| 3 | *PackedB* +
-Suitable for VNNI instructions | *{capability_name}*
-| 4 | *Unused* +
+| 3 | *Unused* +
 Only valid for _TypeJointMatrixINTEL_ | *{capability_name}*


change use names to: A, B, Accumulator
or
MatrixA, matrixB, MatrixAccumulator

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

dkhaldi

LGTM

MrSidims · 2023-02-01T22:57:53Z

Closing as no longer needed

[SPIR-V][DOC] Add 'Use' parameter to TypeJointMatrixINTEL

8592917

'Use' is an optional parameter that shows where in a math operation the matrix is used. It must be the result of a constant instruction with scalar 'integer type'. Signed-off-by: Dmitry Sidorov <[email protected]>

MrSidims requested a review from a team as a code owner April 1, 2022 11:45

MrSidims mentioned this pull request Apr 1, 2022

Add 'Use' parameter to TypeJointMatrixINTEL KhronosGroup/SPIRV-LLVM-Translator#1456

Merged

dkhaldi self-requested a review April 4, 2022 15:24

AlexeySachkov requested changes Aug 10, 2022

View reviewed changes

Apply comments

699248a

Signed-off-by: dmitry.sidorov <[email protected]>

MrSidims force-pushed the private/MrSidims/add-matrix-use branch from 85a18d1 to 699248a Compare August 16, 2022 14:50

MrSidims requested a review from AlexeySachkov August 16, 2022 14:54

bader added spec extension All issues/PRs related to extensions specifications Documentation Missing documentation for the code, compiler or runtime features, etc. SPIR-V Issues related to SPIRV-LLVM-Translator labels Sep 6, 2022

MrSidims added 3 commits September 7, 2022 10:50

Merge remote-tracking branch 'origin/sycl' into private/MrSidims/add-…

6ce20a4

…matrix-use

Address comments and make Use mandatory

41be623

Signed-off-by: Sidorov, Dmitry <[email protected]>

Fix typos and align Scope

4e55f9e

Signed-off-by: Sidorov, Dmitry <[email protected]>

bashbaug reviewed Sep 8, 2022

View reviewed changes

Applied comments

9fb4fd7

Signed-off-by: Sidorov, Dmitry <[email protected]>

MrSidims requested a review from bashbaug October 7, 2022 12:50

PackedA/B -> Packed

3094632

Signed-off-by: Sidorov, Dmitry <[email protected]>

dkhaldi reviewed Oct 13, 2022

View reviewed changes

MrSidims requested a review from gmlueck November 8, 2022 15:23

MrSidims commented Jan 11, 2023

View reviewed changes

sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc Outdated Show resolved Hide resolved

Update sycl/doc/design/spirv-extensions/SPV_INTEL_joint_matrix.asciidoc

1bb44e1

MrSidims mentioned this pull request Jan 11, 2023

[Matrix] Confusing encoding of RowMajor / ColMajor layout #7928

Closed

dkhaldi approved these changes Jan 11, 2023

View reviewed changes

MrSidims closed this Feb 1, 2023

[SPIR-V][DOC] Add 'Use' parameter to TypeJointMatrixINTEL #5944

[SPIR-V][DOC] Add 'Use' parameter to TypeJointMatrixINTEL #5944

Uh oh!

Conversation

MrSidims commented Apr 1, 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!

bashbaug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

dkhaldi left a comment

Choose a reason for hiding this comment

Uh oh!

MrSidims commented Feb 1, 2023

Uh oh!

Uh oh!