Add support for offset load/store by YixingZhang007 · Pull Request #2804 · KhronosGroup/SPIRV-LLVM-Translator

YixingZhang007 · 2024-10-25T02:14:50Z

Add a new form of load/store operations for cooperative matrices that accepts two separate arguments: the row index and the column index. Unlike the original approach requiring a pointer to the matrix base, this new form of load/store operations is expected to yield better optimized code on 2dblock read/write instructions on PVC.

CapabilityCooperativeMatrixOffsetInstructionsINTEL = 6238
OpCooperativeMatrixLoadOffsetINTEL = 6239
OpCooperativeMatrixStoreOffsetINTEL = 6240

CLAassistant · 2024-10-25T02:14:56Z

All committers have signed the CLA.

MrSidims · 2024-10-28T10:04:55Z

test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix_bfloat16_offset.ll

+!4 = !{}
+!5 = !{i16 6, i16 14}
+!6 = !{!"-ze-opt-level=O2"}
+!7 = !{!"ModuleMD", !8, !9, !111, !112, !143, !144, !148, !151, !152, !153, !189, !215, !228, !229, !230, !246, !247, !248, !249, !250, !251, !252, !253, !254, !255, !259, !260, !267, !268, !269, !270, !271, !272, !273, !274, !275, !276, !277, !278, !280, !284, !285, !286, !287, !288, !289, !290, !291, !292, !293, !294, !295, !296, !297, !298, !299, !300, !301, !302, !303, !304, !305, !307, !310, !311, !312, !314, !315, !316, !321}


Please shorten the tests - we won't need all of these metadata

Thanks for the suggestions! I have updated the test.

MrSidims

2 nits, other than that the patch is LGTM

MrSidims · 2024-11-20T13:13:33Z

lib/SPIRV/libSPIRV/SPIRVInstruction.h

 _SPIRV_OP(CooperativeMatrixConstructChecked, true, 8)
 #undef _SPIRV_OP

+class SPIRVJointMatrixOffsetInstructionsINTELInstBase


Suggested change

class SPIRVJointMatrixOffsetInstructionsINTELInstBase

class SPIRVCooperativeMatrixOffsetInstructionsINTELInstBase

Thanks! I have fixed this :)

MrSidims · 2024-11-20T13:13:55Z

lib/SPIRV/libSPIRV/SPIRVInstruction.h

+
+#define _SPIRV_OP(x, ...)                                                      \
+  typedef SPIRVInstTemplate<                                                   \
+      SPIRVJointMatrixOffsetInstructionsINTELInstBase,                         \


I have also fixed this.

MrSidims · 2024-11-20T13:15:04Z

test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix_load_store_offset.ll

@@ -0,0 +1,152 @@
+; This is an adapted copy of test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix.ll


Lets use KHR/SPV_KHR_cooperative_matrix test as the template...

Thanks for the suggestions! I have updated the test :)

MrSidims · 2024-11-20T13:16:25Z

test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix_load_store_offset.ll

+; CHECK-SPIRV: CooperativeMatrixLoadOffsetINTEL [[#MatTy1]]
+; CHECK-SPIRV: CooperativeMatrixLoadOffsetINTEL [[#MatTy2]]
+; CHECK-SPIRV: CooperativeMatrixLoadOffsetINTEL [[#MatTy3]]
+; CHECK-SPIRV: JointMatrixMadINTEL [[#MatTy1]]


...so here we would have CooperativeMatrixMulAddKHR instruction.

Reason: JointMatrixMadINTEL will be deprecated. And real-life SYCL joint_matrix workflows with switch to cooperative matrix KHR when intel/llvm#16045 is merged

Thanks! I have change this.

YixingZhang007 · 2024-12-14T14:02:54Z

@vmaksimo I just did some code clean up. Could you please help review this PR? Thanks for helping :)

MrSidims · 2024-12-16T12:38:21Z

Spec is available as rev 17 of intel/llvm#12497
@vmaksimo may I ask you to take a look at CI issues?

vmaksimo · 2024-12-16T15:10:47Z

CI issue was fixed in main by 4a85dd8, please rebase on top to get CI passing

vmaksimo · 2024-12-16T15:25:13Z

lib/SPIRV/libSPIRV/SPIRVInstruction.h

+_SPIRV_OP(CooperativeMatrixLoadOffset, true, 8, true, 6)
+_SPIRV_OP(CooperativeMatrixStoreOffset, false, 7, true, 7)


Please verify that the last optional operand Memory Operand is really translated as Literal to SPIR-V (there are no checks for that in test, so the bug can be hidden). If no, please apply my suggestion, it should work this way if I'm not misunderstanding

Suggested change

_SPIRV_OP(CooperativeMatrixLoadOffset, true, 8, true, 6)

_SPIRV_OP(CooperativeMatrixStoreOffset, false, 7, true, 7)

_SPIRV_OP(CooperativeMatrixLoadOffset, true, 8, true, 5)

_SPIRV_OP(CooperativeMatrixStoreOffset, false, 7, true, 6)

Hello Viktoria! Thanks so much for the suggestion! I have checked that the last operand Memory Operand is not translated to Literal. I have applied the suggested change :)

… accepts two separate arguments: the row index and the column index.

vmaksimo

LGTM.
Applying the suggestion is not required to get this merged.

test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix_load_store_offset.ll

YixingZhang007 · 2024-12-18T15:48:11Z

@vmaksimo Could you please help merge this PR or if I need to get approval from anyone else? Thanks!

YixingZhang007 marked this pull request as draft October 25, 2024 02:15

MrSidims reviewed Oct 28, 2024

View reviewed changes

YixingZhang007 requested a review from MrSidims November 18, 2024 20:30

YixingZhang007 force-pushed the add_offset_load_store branch from 0bf6ab2 to 47c9ada Compare November 18, 2024 20:33

YixingZhang007 marked this pull request as ready for review November 18, 2024 20:33

MrSidims reviewed Nov 20, 2024

View reviewed changes

YixingZhang007 requested a review from MrSidims December 9, 2024 01:53

YixingZhang007 mentioned this pull request Dec 9, 2024

[SYCL][Joint Matrix] Add support for Offset joint_matrix_load and joi intel/llvm#15526

Draft

MrSidims approved these changes Dec 14, 2024

View reviewed changes

MrSidims requested a review from vmaksimo December 14, 2024 00:32

YixingZhang007 force-pushed the add_offset_load_store branch from ff970c8 to 4dca946 Compare December 14, 2024 13:53

vmaksimo reviewed Dec 16, 2024

View reviewed changes

YixingZhang007 added 8 commits December 16, 2024 08:07

Add a new form of load/store operations for cooperative matrices that…

9950a51

… accepts two separate arguments: the row index and the column index.

code clean up and resolve comment

f6b02f7

code clean up

183bd2d

code clean up

e98123d

update the tokens number

a8409a4

update the test case

d999545

clean up test

9d9615b

code clean up

3fbd371

YixingZhang007 force-pushed the add_offset_load_store branch from 4dca946 to 3fbd371 Compare December 16, 2024 16:07

update the number of literal oprand

e22918e

YixingZhang007 requested a review from vmaksimo December 17, 2024 16:06

update the test

0f4773c

vmaksimo approved these changes Dec 18, 2024

View reviewed changes

test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix_load_store_offset.ll Outdated Show resolved Hide resolved

apply the suggested change

cedd513

vmaksimo merged commit 193661c into KhronosGroup:main Dec 19, 2024
8 checks passed

	class SPIRVJointMatrixOffsetInstructionsINTELInstBase
	class SPIRVCooperativeMatrixOffsetInstructionsINTELInstBase

		@@ -0,0 +1,152 @@
		; This is an adapted copy of test/extensions/INTEL/SPV_INTEL_joint_matrix/joint_matrix.ll

		_SPIRV_OP(CooperativeMatrixLoadOffset, true, 8, true, 6)
		_SPIRV_OP(CooperativeMatrixStoreOffset, false, 7, true, 7)

Conversation

YixingZhang007 commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Oct 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrSidims left a comment

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!

YixingZhang007 commented Dec 14, 2024

Uh oh!

MrSidims commented Dec 16, 2024

Uh oh!

vmaksimo commented Dec 16, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vmaksimo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YixingZhang007 commented Dec 18, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

YixingZhang007 commented Oct 25, 2024 •

edited

Loading

CLAassistant commented Oct 25, 2024 •

edited

Loading