[AIE2P] Refactor register bank selection #325

niwinanto · 2025-01-31T10:51:26Z

No description provided.

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

martien-de-jong · 2025-02-10T08:59:59Z

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

-                           const MachineInstr &MI, const Register &AccReg) {
+static bool isUsedAsAccRegInIntrinsic(const MachineRegisterInfo &MRI,
+                                      const MachineInstr &MI,
+                                      const Register &AccReg) {
  switch (cast<GIntrinsic>(MI).getIntrinsicID()) {
  // All Intrinsics with accumlator destination operand


nit: accumulator + one more below

martien-de-jong · 2025-02-10T09:09:17Z

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

  unsigned Op = MI.getOpcode();
  switch (Op) {
  default:
    break;
  case TargetOpcode::G_INTRINSIC:
  case TargetOpcode::G_INTRINSIC_W_SIDE_EFFECTS:
-    return isAccIntrinsic(MRI, MI, RegOp);
+    return isUsedAsAccRegInIntrinsic(MRI, MI, Reg);
  case TargetOpcode::COPY: {
    Register DstReg = MI.getOperand(0).getReg();
    if (isAccReg(DstReg))


What is the difference between isAccReg and getRegBank(Reg) == AccRegBank below?

As I understand, isAccReg look for a register is a physical accumulator register and getRegBank(Reg) == AccRegBank looks for AccRegBank is assigned for a virtual register.

martien-de-jong · 2025-02-10T09:11:32Z

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

-  auto traceToActualReg = [&](Register &Reg) {
-    for (auto &UseMI : MRI.use_nodbg_instructions(Reg)) {
-      MachineInstr *ConvUseMI = &UseMI;
+  auto TraceToActualReg = [&](Register &SkipReg) {


Could we pass by value and return the result?

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

martien-de-jong · 2025-02-10T09:19:51Z

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

      isAccRegMapping = true;
-    if (isUseFifoInsn(MRI, TRI, UseCandidate))
+    if (&AIE2P::FifoRegBank == PreferredRB)


nit: else if?

andcarminati · 2025-02-13T10:49:16Z

llvm/lib/Target/AIE/aie2p/AIE2PRegisterBankInfo.h

+                                     const MachineRegisterInfo &MRI,
+                                     const TargetRegisterInfo &TRI,
+                                     Register FifoReg);
+  typedef bool (*RegisterUsedAsSpecificBankFcnPtr)(


I think this is too old for C++. We can use modern alternatives, like std::function.

andcarminati · 2025-02-13T11:00:09Z

llvm/lib/Target/AIE/aie2p/AIE2PRegisterBankInfo.h

-  bool isUseFifoInsn(const MachineRegisterInfo &MRI,
-                     const TargetRegisterInfo &TRI, const Register FifoReg,
-                     unsigned Depth = 0) const;
+  static bool isUsedAsAccRegInInstr(const MachineInstr &MI,


Static in .h files is unusual. I suggest to keep as is.

The reason I changed to static is, non-static member functions are not allowed to pass. I can remove the declarations.

andcarminati · 2025-02-13T11:46:49Z

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

+const RegisterBank *AIE2PRegisterBankInfo::getPreferredRegBankForVectorTy(
+    const MachineRegisterInfo &MRI, const TargetRegisterInfo &TRI,
+    Register Reg) const {
+  auto RegTy = MRI.getType(Reg);


nit: use the real type here for both variables.

andcarminati · 2025-02-13T11:49:16Z

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

-    }
-  }
-  return false;
+  if (registerBankCheckFunc(isUsedAsAccRegInInstr, MRI, TRI, Reg))


Maybe we can use a more assertive name here, like registerBankLookAheadSearch.

andcarminati · 2025-02-13T11:56:54Z

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

+    return &getRegBank(AIE2P::AccRegBankID);
+  if (registerBankCheckFunc(isUsedAsFifoRegInInstr, MRI, TRI, Reg))
+    return &getRegBank(AIE2P::FifoRegBankID);
+  return &getRegBank(AIE2P::VRegBankID);


Do we check against VRegBank? Or do we always check against Acc and Fifo? A feel dangerous just return VRegBank. We don't have a "Register holds vector type" precondition here.

llvm/lib/Target/AIE/aie2p/AIE2PRegisterBankInfo.h

andcarminati · 2025-02-13T16:08:30Z

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

+    return &getRegBank(AIE2P::AccRegBankID);
+  else if (registerBankLookAheadSearch(isUsedAsFifoRegInInstr, MRI, TRI, Reg))
+    return &getRegBank(AIE2P::FifoRegBankID);
+  else if (RegSize > 64)


This is a very cheap check. Can we anticipate it? Like:

if (RegSize <= 256 && RegSize > 64) return &getRegBank(AIE2P::VRegBankID)

andcarminati · 2025-02-13T16:10:08Z

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

@@ -841,7 +837,9 @@ AIE2PRegisterBankInfo::getInstrMapping(const MachineInstr &MI) const {
      auto *RB = getRegBank(DstReg, MRI, TRI);
      if (RB == &AIE2P::AccRegBank)
        return AIEBaseRegisterBankInfo::getInstrMapping(MI);
-      if (isUseAccInsn(MRI, TRI, DstReg)) {
+      auto PreferredRegBank = getPreferredRegBankForVectorTy(MRI, TRI, DstReg);
+      if (PreferredRegBank.has_value() &&


nit: just if (PreferredRegBank && &AIE2P::AccRegBank == *PreferredRegBank).

Below as well.

andcarminati

LGTM. Nice work!

niwinanto force-pushed the niwin.regbank.select branch from e68a4c6 to 3f14d94 Compare January 31, 2025 11:10

andcarminati reviewed Feb 4, 2025

View reviewed changes

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

andcarminati reviewed Feb 4, 2025

View reviewed changes

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

niwinanto force-pushed the niwin.regbank.select branch from 3f14d94 to 9ac5133 Compare February 5, 2025 11:16

niwinanto marked this pull request as ready for review February 5, 2025 11:22

niwinanto requested review from abhinay-anubola, abnikant, F-Stuckmann, gbossu, katerynamuts, khallouh, konstantinschwarz, martien-de-jong, SagarMaheshwari99 and stephenneuendorffer as code owners February 5, 2025 11:22

niwinanto changed the title ~~[WIP][AIE2P] Refactor register bank selection~~ [AIE2P] Refactor register bank selection Feb 5, 2025

khallouh reviewed Feb 5, 2025

View reviewed changes

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

khallouh reviewed Feb 5, 2025

View reviewed changes

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

martien-de-jong reviewed Feb 10, 2025

View reviewed changes

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

martien-de-jong reviewed Feb 10, 2025

View reviewed changes

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

martien-de-jong reviewed Feb 10, 2025

View reviewed changes

andcarminati mentioned this pull request Feb 10, 2025

[AIE2P] Optimize G_PHI regbankselect for fifo/acc #314

Merged

niwinanto force-pushed the niwin.regbank.select branch from 9ac5133 to f9b7212 Compare February 11, 2025 20:26

andcarminati reviewed Feb 13, 2025

View reviewed changes

niwinanto force-pushed the niwin.regbank.select branch from f9b7212 to c5b401a Compare February 13, 2025 11:33

andcarminati reviewed Feb 13, 2025

View reviewed changes

llvm/lib/Target/AIE/aie2p/AIE2PRegisterBankInfo.h Outdated Show resolved Hide resolved

niwinanto force-pushed the niwin.regbank.select branch 4 times, most recently from 3555c8c to 8afe399 Compare February 13, 2025 14:37

andcarminati reviewed Feb 13, 2025

View reviewed changes

niwinanto force-pushed the niwin.regbank.select branch 2 times, most recently from 4e26531 to 54e9f3e Compare February 13, 2025 16:29

[AIE2P] Refactor register bank selection for finding acc/fifo bank

d2ba106

niwinanto force-pushed the niwin.regbank.select branch from 54e9f3e to d2ba106 Compare February 13, 2025 16:31

andcarminati approved these changes Feb 13, 2025

View reviewed changes

niwinanto enabled auto-merge (rebase) February 13, 2025 16:40

niwinanto merged commit a0b3ab2 into aie-public Feb 13, 2025
8 checks passed

SagarMaheshwari99 deleted the niwin.regbank.select branch February 14, 2025 08:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIE2P] Refactor register bank selection #325

[AIE2P] Refactor register bank selection #325

niwinanto commented Jan 31, 2025

martien-de-jong Feb 10, 2025 •

edited

Loading

martien-de-jong Feb 10, 2025

niwinanto Feb 13, 2025

martien-de-jong Feb 10, 2025

martien-de-jong Feb 10, 2025

andcarminati Feb 13, 2025

andcarminati Feb 13, 2025

niwinanto Feb 13, 2025 •

edited

Loading

andcarminati Feb 13, 2025

andcarminati Feb 13, 2025

andcarminati Feb 13, 2025

andcarminati Feb 13, 2025 •

edited

Loading

andcarminati Feb 13, 2025 •

edited

Loading

andcarminati left a comment

[AIE2P] Refactor register bank selection #325

[AIE2P] Refactor register bank selection #325

Conversation

niwinanto commented Jan 31, 2025

martien-de-jong Feb 10, 2025 • edited Loading

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

niwinanto Feb 13, 2025 • edited Loading

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

andcarminati Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

andcarminati Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

andcarminati left a comment

Choose a reason for hiding this comment

martien-de-jong Feb 10, 2025 •

edited

Loading

niwinanto Feb 13, 2025 •

edited

Loading

andcarminati Feb 13, 2025 •

edited

Loading

andcarminati Feb 13, 2025 •

edited

Loading