[PATCH] D90304: [GlobalISel] Introduce optimal variant of regbankselect

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/RegBankSelect.h:103-105
+    /// Find optimal solution. This should increase code quality,
+    /// but at the cost of longer compile time.
+    Optimal
----------------
I think calling this "optimal" is a bit  overly optimistic. Maybe Global?


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1046
+bool isMachineInstrWithRegClasses(const MachineInstr &MI) {
+  return (isTargetSpecificOpcode(MI.getOpcode()) && !MI.isPreISelOpcode()) ||
+    MI.isInlineAsm();
----------------
!isPreIselOpcode should cover asm already?


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1051
+static inline
+bool isMachineOperandWithVirtualReg(MachineOperand MO) {
+  return MO.isReg() && Register::isVirtualRegister(MO.getReg());
----------------
arsenm wrote:
> Can do just .getReg().isVirtual()
This name is a bit obnoxious. If it's really necessary, I would rather just add this helper directly to MachineOperand


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1051-1053
+bool isMachineOperandWithVirtualReg(MachineOperand MO) {
+  return MO.isReg() && Register::isVirtualRegister(MO.getReg());
+}
----------------
Can do just .getReg().isVirtual()


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1146-1147
+  for (MachineBasicBlock *MBB : post_order(&MF)) {
+    if (MBB->empty())
+      continue;
+    bool ReachedBegin = false;
----------------
This explicit empty check is redundant


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1175
+
+      MachineIRBuilder MIB(*MBB, std::next(MII)); // Insert new instructions
+                                                  // after the current MI.
----------------
Should not construct a fresh MachineIRBuilder every time


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1178-1179
+      unsigned NumInserted = 0;
+      assignRegBank(MI, MIB, NumInserted);
+      MII = std::next(MII, NumInserted);
+    }
----------------
Is this intended to allow applyMappingImpl to insert new instructions which will iteratively have banks assigned? If so I think having a feature disparity with the single pass path is a problem


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1448
+                                  unsigned &NumInserted) {
+  LLVM_DEBUG(dbgs() << "- Processing " << MI);  // Implicit \n.
+
----------------
Comment is unnecessary


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1450
+
+  auto &GSI = getGSIOfMI(MI);
+  SmallVector<std::tuple<MachineInstr *, unsigned>, 4> MIsToUpdate;
----------------
Don't like this getGSIOfMI name


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1545
+    // Assign register bank to result of instruction
+    if (CurMI->getNumExplicitDefs() > 0) {
+      MachineOperand &DestMO = CurMI->getOperand(0);
----------------
This checks for multiple explicit defs, but then seems to assume only a single def


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90304/new/

https://reviews.llvm.org/D90304