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

[Gabriel Hjort Åkerlund via Phabricator via llvm-commits]

ehjogab added a comment.

Thank you very much for your feedback, arsenm! Really appreciate it!

I noticed a crapload of lint warnings so I will fix them first, then fix your comments that I agree with, and then we can discuss further on those that I don't. =)



================
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
----------------
arsenm wrote:
> I think calling this "optimal" is a bit  overly optimistic. Maybe Global?
With respect to the cost function, it should be optimal. I don't have a detailed proof yet, but the intuition is that it computes the cost of putting a certain register in a certain register bank, and by selecting the choice with lowest cost and working its way backwards to achieve that, it really should give the optimal solution. However, there is a stage in the algorithm where it may need to rematerialize the instruction (a certain situation when there exist multiple usages), and there I'm not sure whether optimality is kept...


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1046
+bool isMachineInstrWithRegClasses(const MachineInstr &MI) {
+  return (isTargetSpecificOpcode(MI.getOpcode()) && !MI.isPreISelOpcode()) ||
+    MI.isInlineAsm();
----------------
arsenm wrote:
> !isPreIselOpcode should cover asm already?
Don't know; I just copied what was already done in greedy, so you may be right...


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1051
+static inline
+bool isMachineOperandWithVirtualReg(MachineOperand MO) {
+  return MO.isReg() && Register::isVirtualRegister(MO.getReg());
----------------
arsenm wrote:
> 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
I agree, it's not a good name. I'll see if I can either get rid of this function, or else put it directly in MachineOperand.


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1051-1053
+bool isMachineOperandWithVirtualReg(MachineOperand MO) {
+  return MO.isReg() && Register::isVirtualRegister(MO.getReg());
+}
----------------
ehjogab wrote:
> arsenm wrote:
> > 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
> I agree, it's not a good name. I'll see if I can either get rid of this function, or else put it directly in MachineOperand.
You mean this works even if MO is not actually a register? I just assumed that you're not allowed to invoke getReg() unless you are certain that MO is really a register.


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


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1175
+
+      MachineIRBuilder MIB(*MBB, std::next(MII)); // Insert new instructions
+                                                  // after the current MI.
----------------
arsenm wrote:
> Should not construct a fresh MachineIRBuilder every time
I need to have a new position of insertion for each instruction, hence the creation of a new MachineIRBuilder each time. Is it possible to keep the same MachineIRBuilder and simply change the insertion point, without affecting already inserted instructions?


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1178-1179
+      unsigned NumInserted = 0;
+      assignRegBank(MI, MIB, NumInserted);
+      MII = std::next(MII, NumInserted);
+    }
----------------
arsenm wrote:
> 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
Are you saying that I should rewrite the code to make use of applyMappingImpl, such that both single- and multi-pass make use of it? Because currently the multi-pass doesn't use it at all.


================
Comment at: llvm/lib/CodeGen/GlobalISel/RegBankSelect.cpp:1306
+      auto MO = MI.getOperand(i);
+      if (!isMachineOperandWithVirtualReg(MO)) {
+        continue;
----------------
arsenm wrote:
> I think checking if this is virtual is redundant; any instruction subject to rgebank select should not have physical register operands
For uniformity, I decided to treat all instructions (except debug, which are skipped) equally in order to preserve the cost chain and register bank preferences. Hence this step will look at instructions which by themselves are not subject to regbankselect, but they may produce results which are used by instructions that are.


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


================
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);
----------------
arsenm wrote:
> This checks for multiple explicit defs, but then seems to assume only a single def
Hm, true. The code below assumes that there's exactly one such def, and if there are multiple explicit defs it should crash as it cannot handle such cases at the moment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90304