[AIE2P] Implemented VST.PACK combine #280

katerynamuts · 2025-01-17T13:33:46Z

No description provided.

andcarminati · 2025-01-17T15:43:18Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+      CombOpIntrinsicID != Intrinsic::aie2p_pack_I1024_I4_I8)
+    return {};
+
+  assert((getLoadStoreSize(MemOp) == 256) && "Unexpected STS.SRS size");


nit: VST.PACK

andcarminati · 2025-01-17T15:56:26Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+
+  auto Opcode = TII.getMvSclMultiSlotPseudoOpcode();
+  MIB.setInstr(*NewInstr);
+  MIB.buildInstr(Opcode, {AIE2P::crPackSize}, {}).addImm((unsigned)Is8Bit);


Oh, this crPackSize usage is very subtle in the documentation.

This is not the only place were we have to set a control register. Can we create a function for this in AIEBaseInstructionSelector.cpp, as we did with setUnsetCtrlRegister?

andcarminati · 2025-01-17T15:57:02Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+  // Selects the size of the Pack instructions
+  // 0 – Destination is 4 bits
+  // 1 – Destination is 8 bits
+  bool Is8Bit = cast<GIntrinsic>(PackOp)->getIntrinsicID() ==


nit: const.

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-postinc-2d-vst_pack.mir

gbossu · 2025-01-20T14:31:00Z

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-indexed-vst_pack.mir

+    ; CHECK-NEXT: $packsign0 = MOV_scalar_imm11_pseudo 0
+    ; CHECK-NEXT: [[COPY3:%[0-9]+]]:edj = COPY $dj0
+    ; CHECK-NEXT: $crpacksize = MOV_scalar_imm11_pseudo 1
+    ; CHECK-NEXT: $packsign0 = COPY [[COPY2]]


Curious: We have two packSign registers in AIE2p? I assume both are addressible, and here this is just an arbitrary choice to use packsign0 for the dynamic case?

yes, we have two packSign registers in AIE2p, packSign0 and packSign1. We have the same situation for vaddSign, unpackSign, upsSign, srsSign. In all cases for the dynamic case we just use the instruction with the xxxSign0.

gbossu

LGTM, I couldn't spot something wrong. I have checked about half of the tests, and trust the rest are correct as well. Up to you if you want to seek another review from AIE2p core team :)
Before merging, maybe have a QoR run to check those combines are used in practice and do not introduce new failures.

mludevid

Please go through all test files and fix the types and sizes of all vectors involved.

aie2p_pack_I512_I8_I16: the input vector should be <32 x s16> and the output one <32 x s8>
aie2p_pack_I512_I4_I8: the input vector should be <64 x s8> and the output one <64 x s4>
aie2p_pack_I1024_I8_I16: the input vector should be <64 x s16> and the output one <64 x s8>
aie2p_pack_I1024_I4_I8: the input vector should be <128 x s8> and the output one <128 x s4>

mludevid · 2025-01-22T09:30:43Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

Could you update the header of this file to 2024-2025?

mludevid · 2025-01-22T09:49:08Z

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp

Please update this header as well: 2023-2025

mludevid · 2025-01-22T09:59:33Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+  if (CombOp.getOpcode() != AIE2P::G_INTRINSIC_W_SIDE_EFFECTS)
+    return false;


This check is duplicated, we also check this in getCombinedOpcodePACK. I would prefer to just keep it in getCombinedOpcodePACK.

mludevid · 2025-01-22T10:09:28Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+  assert((MemOpLoadStoreSize == 256 &&
+          (SrcRegSize == 512 || SrcRegSize == 1024)) &&
+         "Unexpected VST.PACK size");


In AIE2P the store size can be 512 bits if the source register size is 1024. I would suggest updating the assert to:

assert((MemOpLoadStoreSize == 256 && SrcRegSize == 512) || (MemOpLoadStoreSize == 512 && SrcRegSize == 1024))

mludevid · 2025-01-22T10:10:38Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+      CombOpIntrinsicID != Intrinsic::aie2p_pack_I1024_I4_I8)
+    return {};
+
+  assert((getLoadStoreSize(MemOp) == 256) && "Unexpected VST.PACK size");


As mentioned in another comment the memory size can either be 256 or 512 in AIE2P.

mludevid · 2025-01-22T10:19:56Z

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp

+
+  auto Opcode = TII.getMvSclMultiSlotPseudoOpcode();
+  MIB.setInstr(*NewInstr);
+  MIB.buildInstr(Opcode, {AIE2P::crPackSize}, {}).addImm((unsigned)Is8Bit);


This is not the only place were we have to set a control register. Can we create a function for this in AIEBaseInstructionSelector.cpp, as we did with setUnsetCtrlRegister?

katerynamuts requested review from abhinay-anubola, abnikant, andcarminati, gbossu, khallouh, konstantinschwarz, martien-de-jong, SagarMaheshwari99 and stephenneuendorffer as code owners January 17, 2025 13:33

andcarminati reviewed Jan 17, 2025

View reviewed changes

katerynamuts force-pushed the kmuts.vst_pack branch from 2df64ac to cb2fa84 Compare January 20, 2025 08:28

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/lib/Target/AIE/AIE2InstructionSelector.cpp Show resolved Hide resolved

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp Show resolved Hide resolved

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PInstructionSelector.cpp Outdated Show resolved Hide resolved

gbossu reviewed Jan 20, 2025

View reviewed changes

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-postinc-2d-vst_pack.mir Show resolved Hide resolved

katerynamuts force-pushed the kmuts.vst_pack branch from cb2fa84 to 81ada5d Compare January 20, 2025 13:59

katerynamuts requested a review from gbossu January 20, 2025 13:59

gbossu reviewed Jan 20, 2025

View reviewed changes

gbossu previously approved these changes Jan 20, 2025

View reviewed changes

katerynamuts force-pushed the kmuts.vst_pack branch 3 times, most recently from 117c802 to f2e1b63 Compare January 21, 2025 09:46

mludevid self-requested a review January 22, 2025 08:58

mludevid reviewed Jan 22, 2025

View reviewed changes

mludevid mentioned this pull request Jan 22, 2025

[AIE2P] Implemented VLDB.UNPACK combine #299

Merged

katerynamuts dismissed gbossu’s stale review via 2535058 January 24, 2025 10:31

katerynamuts force-pushed the kmuts.vst_pack branch from f2e1b63 to 2535058 Compare January 24, 2025 10:31

katerynamuts requested review from F-Stuckmann and niwinanto as code owners January 24, 2025 10:31

gbossu approved these changes Jan 24, 2025

View reviewed changes

[AIE2P] Implemented VST.PACK combine

a9c1fc9

katerynamuts force-pushed the kmuts.vst_pack branch from 2535058 to a9c1fc9 Compare January 24, 2025 14:28

gbossu approved these changes Jan 24, 2025

View reviewed changes

katerynamuts merged commit 44fec18 into aie-public Jan 24, 2025
8 checks passed

katerynamuts deleted the kmuts.vst_pack branch January 24, 2025 16:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P] Implemented VST.PACK combine #280

[AIE2P] Implemented VST.PACK combine #280

katerynamuts commented Jan 17, 2025

andcarminati Jan 17, 2025

andcarminati Jan 17, 2025

katerynamuts Jan 20, 2025

mludevid Jan 22, 2025

andcarminati Jan 17, 2025

gbossu Jan 20, 2025

katerynamuts Jan 20, 2025

gbossu left a comment

mludevid left a comment

mludevid Jan 22, 2025

mludevid Jan 22, 2025

mludevid Jan 22, 2025

mludevid Jan 22, 2025

mludevid Jan 22, 2025

mludevid Jan 22, 2025

		if (CombOp.getOpcode() != AIE2P::G_INTRINSIC_W_SIDE_EFFECTS)
		return false;

[AIE2P] Implemented VST.PACK combine #280

[AIE2P] Implemented VST.PACK combine #280

Conversation

katerynamuts commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbossu left a comment

Choose a reason for hiding this comment

mludevid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment