Skip to content

Align SPV_INTEL_joint_matrix implementation to the spec#3468

Open
vmaksimo wants to merge 2 commits intoKhronosGroup:mainfrom
vmaksimo:joint-matrix-align-to-last-spec
Open

Align SPV_INTEL_joint_matrix implementation to the spec#3468
vmaksimo wants to merge 2 commits intoKhronosGroup:mainfrom
vmaksimo:joint-matrix-align-to-last-spec

Conversation

@vmaksimo
Copy link
Contributor

@vmaksimo vmaksimo commented Dec 4, 2025

Spec: intel/llvm#12497

Change summary:

  • added Packed matrix layout to support Intel VNNI instructions.
  • remove JointMatrixGetElementCoord in favor of the same cooperative matrix instruction.
  • support new Cooperative Matrix Operands to specify component type interpretation for tf32 and bfloat16 types.

Spec: intel/llvm#12497

Change summary:
* added Packed matrix layout to support Intel VNNI instructions.
* remove `JointMatrixGetElementCoord` in favor of the same cooperative matrix instruction.
* support new Cooperative Matrix Operands to specify component type interpretation for tf32 and bfloat16 types.
Copy link
Contributor

@YuriPlyakhin YuriPlyakhin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Provided one nit comment above. Also, a question: should we first merge intel/llvm#12497 before merging translator changes?

@vmaksimo
Copy link
Contributor Author

vmaksimo commented Dec 8, 2025

LGTM. Provided one nit comment above. Also, a question: should we first merge intel/llvm#12497 before merging translator changes?

Shortly - we should not.
But this is a question for @MrSidims - I'm not sure what is the process with the spec merge, my guess is that we'll merge it once 100% finalized.

@MrSidims
Copy link
Contributor

MrSidims commented Dec 8, 2025

Also, a question: should we first merge intel/llvm#12497 before merging translator changes?

There is no such requirement. Most important thing - the spec is public.

typedef SPIRVInstTemplate<SPIRVJointMatrixINTELWorkItemInst, \
internal::Op##x##INTEL, __VA_ARGS__> \
SPIRV##x##INTEL;
_SPIRV_OP(JointMatrixGetElementCoord, true, 5)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To double check, what instruction is used in SYCL headers right now?

Copy link
Contributor Author

@vmaksimo vmaksimo Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then I'd suggest to first replace the joint matrix version to cooperative matrix version in the headers and see if anything fails.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a draft PR: intel/llvm#20855

; RUN: llvm-as < %s -o %t.bc
; RUN: llvm-spirv %t.bc --spirv-ext=+SPV_KHR_cooperative_matrix,+SPV_INTEL_joint_matrix -o %t.spv
; RUN: llvm-spirv %t.spv -to-text -o %t.spt
; RUN: FileCheck < %t.spt %s --check-prefix=CHECK-SPIRV
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a spirv-val check? Or is not prepared yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe SPIR-V Tools don't support SPV_INTEL_joint_matrix at all - and not sure if this work planned by us any time soon or at all (@MrSidims I guess it also affects validation for SPIR-V BE, but it's not critical?)

; CHECK-SPIRV-DAG: Extension "SPV_KHR_cooperative_matrix"
; CHECK-SPIRV-DAG: Extension "SPV_INTEL_joint_matrix"
; CHECK-SPIRV-DAG: Capability CooperativeMatrixBFloat16ComponentTypeINTEL
; 64 stays for MatrixAAndBBFloat16ComponentsINTEL (0x40)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also test 0x80 and 0x100? I don't see them tested now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just aligned to the tests made in SPIR-VE BE (they also test only this case). Not sure we must add more at this point

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants