[AIE2P] Support Wide Load/Store #307

abnikant · 2025-01-23T13:23:35Z

Refactor 1024-bit FIFO Load/Store.
Support 1024-bits and 2048-bits (accumulator) Load/Store.
Add tests.

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

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-load.mir

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-store.mir

niwinanto · 2025-01-24T05:04:31Z

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-store.mir

+    ; CHECK-NEXT: [[COPY2:%[0-9]+]]:vec512 = COPY [[COPY1]].sub_512_lo
+    ; CHECK-NEXT: VST_dmx_sts_x_idx_imm [[COPY2]], [[COPY]], 0 :: (store (<32 x s16>), align 128)
+    ; CHECK-NEXT: [[COPY3:%[0-9]+]]:vec512 = COPY [[COPY1]].sub_512_hi
+    ; CHECK-NEXT: VST_dmx_sts_x_idx_imm [[COPY3]], [[COPY]], 64 :: (store (<32 x s16>) into unknown-address + 64)


Just a clarification, into unknown-address what does it mean.

since the ir element which point to the address is removed from the test (G_LOAD/G_STORE) to make the test simple.

niwinanto · 2025-01-24T05:13:09Z

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-store.mir

+    ; CHECK-NEXT: VST_dmx_sts_bm_idx_imm [[COPY2]], [[COPY]], 0 :: (store (<16 x s32>), align 256)
+    ; CHECK-NEXT: [[COPY3:%[0-9]+]]:acc512 = COPY [[COPY1]].sub_512_acc_hi
+    ; CHECK-NEXT: VST_dmx_sts_bm_idx_imm [[COPY3]], [[COPY]], 64 :: (store (<16 x s32>) into unknown-address + 64)
+    ; CHECK-NEXT: [[COPY4:%[0-9]+]]:acc512 = COPY [[COPY1]].sub_1024_acc_hi_then_sub_512_acc_lo


sub_1024_acc_hi_then_sub_512_acc_lo is it legal? Does it reach till assembly generation?

niwinanto · 2025-01-24T05:28:50Z

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-load.mir

+    ; CHECK: liveins: $p0
+    ; CHECK-NEXT: {{  $}}
+    ; CHECK-NEXT: [[COPY:%[0-9]+]]:ep = COPY $p0
+    ; CHECK-NEXT: [[VLDA_dmx_lda_x_idx_imm:%[0-9]+]]:vec512 = VLDA_dmx_lda_x_idx_imm [[COPY]], 0 :: (load (<32 x s16>), align 128)


As I understands, alignment for 512, 1024, 2048 vector load is 64, 128 and 256. Right?

niwinanto · 2025-01-24T05:42:37Z

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

@@ -103,6 +103,9 @@ class AIE2PInstructionSelector : public AIEBaseInstructionSelector {
  bool select1024BitG_AIE_LOAD_STORE(MachineInstr &I, LoadStoreOpcodes &LSO,


I can see this function has renamed, would you like to remove this?

niwinanto · 2025-01-24T05:57:16Z

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

+    for (unsigned i = 0; i < NumSplits; ++i) {
+      unsigned Offset = (SrcDstTySize == 2048 && i == 0) ? 128 : 0;
+      addSplitMemOperands(
+          AMI.MemI, Instrs[SplitFactor - 1 - 2 * i] /*Higher MIB*/,


Could you explain this SplitFactor - 1 - 2 * i, I see for SplitFactor = 2, this can be even negative value.

but the loop is running for NumSplits - 1 times, and NumSplits = SplitFactor / 2.

It will be nice to have a comment about this lambda.

niwinanto · 2025-01-24T06:03:59Z

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

-           RBI.constrainGenericRegister(SrcDstReg, *&AIE2P::FIFO1024RegClass,
-                                        MRI);
+    if (!RBI.constrainGenericRegister(
+            SrcDstReg, *(SplitFactor == 4 ? RC2048 : RC1024), MRI))


Nit: I would suggest SrcDstTySize == 2048 for more readability.

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

niwinanto · 2025-01-24T06:10:52Z

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

    MachineInstr &I, LoadStoreOpcodes &LSO, AddressingModeInfo &AMI,
    MachineRegisterInfo &MRI) {
+  LLT SrcDstTy = MRI.getType(AMI.SrcDstOp.getReg());
+  unsigned SrcDstTySize = SrcDstTy.getSizeInBits();


Please add an assert for not misusing this function.

it's there, check , we are handling only for supported RBID else it's unreachable.

niwinanto · 2025-01-24T06:23:52Z

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

@@ -2055,6 +2069,14 @@ LoadStoreOpcodes AIE2PInstructionSelector::getLoadStoreOpcode(
      }
      llvm_unreachable("1024-bit vector type must be in AccRegBank or "
                       "VRegBank or FifoRegBankID");
+    } else if (LoadStoreSize == 2048) {
+      assert(RBID == AIE2P::AccRegBankID &&


Why we are not handling if (PtrDef->getOpcode() == AIE2P::G_FRAME_INDEX) { case

niwinanto · 2025-01-24T06:28:06Z

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

+      }
+      if (RBID == AIE2P::VRegBankID) {
+        return {/*ISelOpcode=*/AIE2P::VLDA_dmx_lda_x_idx_imm,
+                /*FitsImmediateRange=*/true,


Nit: AlwaysFitsImmediateRange

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

khallouh · 2025-01-24T08:33:24Z

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

+      int Offset = SubReg * 64;
+      auto Copy = MIB.buildInstr(TargetOpcode::COPY, {SubRegs[SubReg]}, {})
+                      .addReg(AMI.SrcDstOp.getReg(), 0,
+                              SubRegIdxes[SubReg % SubRegIdxes.size()]);


How about the following instead?

.addReg(AMI.SrcDstOp.getReg(), 0, SubRegIdxes[SubRegIdx]);

Since SubRegIdx is always less than SplitFactor in the loop

andcarminati · 2025-01-24T08:49:47Z

Looking to the PR I see no handling for G_AIE_OFFSET_LOAD or other postinc variants of load and stores. Is there any plans to handle those cases?

abnikant · 2025-01-24T09:17:35Z

Looking to the PR I see no handling for G_AIE_OFFSET_LOAD or other postinc variants of load and stores. Is there any plans to handle those cases?

yes, I am working on it. Will open other PR to support it.

abnikant · 2025-01-24T14:34:12Z

@andcarminati , @khallouh @niwinanto , The comments are addressed . Please check if you have any more comments.

martien-de-jong · 2025-01-27T14:47:50Z

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

+      addSplitMemOperands(
+          AMI.MemI, Instrs[SplitFactor - 1 - 2 * I] /*Higher MIB*/,
+          Instrs[SplitFactor - 2 - 2 * I] /*Lower MIB*/, Offset, SplitFactor);
+    }


why not

if (SrcDstType == 2048) { addSplitMemOperands() addSplitMemOperands() } else { addSplitMemOperands() }

and make the comment superfluous?

yes, thanks for looking into it @martien-de-jong . Updated the code.

martien-de-jong · 2025-01-27T15:06:13Z

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

+      const unsigned Offset = SubRegIdx * 64;
+      auto Copy = MIB.buildInstr(TargetOpcode::COPY, {SubRegs[SubRegIdx]}, {})
+                      .addReg(AMI.SrcDstOp.getReg(), 0,
+                              SubRegIdxes[SubRegIdx % SubRegIdxes.size()]);


Right. SubRegIdx is an index into the SubRegs array, not to be confused with an index into the SubRegsIdxes array.

niwinanto

LGTM!

khallouh

LGTM

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

khallouh reviewed Jan 23, 2025

View reviewed changes

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

khallouh reviewed Jan 23, 2025

View reviewed changes

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-load.mir Show resolved Hide resolved

khallouh reviewed Jan 23, 2025

View reviewed changes

llvm/test/CodeGen/AIE/aie2p/GlobalIsel/inst-select-vector-store.mir Show resolved Hide resolved

niwinanto reviewed Jan 24, 2025

View reviewed changes

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

niwinanto reviewed Jan 24, 2025

View reviewed changes

abnikant force-pushed the aie2p.wide.load.store branch 2 times, most recently from 024512c to 6aac9bb Compare January 24, 2025 07:29

andcarminati reviewed Jan 24, 2025

View reviewed changes