[GISel]: Refactor InstructionSelector::constrainSelectInstOperands

[aditya_nandakumar]

Summary

This patch moves the constrainSelectedInstRegOperands from InstructionSelector into GlobalISel/Utils so it can be used elsewhere.
Also added constrainAllUses method into MachineInstrBuilder which forwards the call to utils. So now we can use it like,

BuildMI(...)

.addUse(..)
.addImm(..)
.constrainAllUses(TII, TRI, RBI);

[dsanders]

Hi Aditya,

You forgot to include llvm-commits as a subscriber. Phabricator won't send the patch to the list if you add it now so you'll need to abandon this revision and create a new one.

The subject line '[GISel]: Refactor InstructionSelector::constrainSelectInstOperands' doesn't really describe what you're trying to do. It says what changed but not what effect that change has. Something like '[GISel] Make constrainSelectedInstRegOperands() available to the legalizer. NFC' would be better.

Other than that, this looks good to me. If you re-post the patch but including llvm-commits, I can LGTM it on-list.

[dsanders]

In D42149#978627, @dsanders wrote:

Hi Aditya,

You forgot to include llvm-commits as a subscriber. Phabricator won't send the patch to the list if you add it now so you'll need to abandon this revision and create a new one.
<snip>
Other than that, this looks good to me. If you re-post the patch but including llvm-commits, I can LGTM it on-list.

Sorry, ignore that bit. You did include llvm-commits, it's just that Phabricator has moved where the subscribers are listed. They used to be just under the reviewers but now they're off to the side.

LGTM with the subject line change.

[aditya_nandakumar]

Thanks Daniel. Committed in r322743.

include/llvm/CodeGen/GlobalISel/InstructionSelector.h

protected:
   /// after (for defs) and may replace the operand of \p I.
   /// \returns whether operand regclass constraining succeeded.
   bool constrainOperandRegToRegClass(MachineInstr &I, unsigned OpIdx,
                                      const TargetRegisterClass &RC,
                                      const TargetInstrInfo &TII,
                                      const TargetRegisterInfo &TRI,
                                      const RegisterBankInfo &RBI) const;
 
-  /// Mutate the newly-selected instruction \p I to constrain its (possibly
-  /// generic) virtual register operands to the instruction's register class.
-  /// This could involve inserting COPYs before (for uses) or after (for defs).
-  /// This requires the number of operands to match the instruction description.
-  /// \returns whether operand regclass constraining succeeded.
-  ///
-  // FIXME: Not all instructions have the same number of operands. We should
-  // probably expose a constrain helper per operand and let the target selector
-  // constrain individual registers, like fast-isel.
-  bool constrainSelectedInstRegOperands(MachineInstr &I,
-                                        const TargetInstrInfo &TII,
-                                        const TargetRegisterInfo &TRI,
-                                        const RegisterBankInfo &RBI) const;
-
   bool isOperandImmEqual(const MachineOperand &MO, int64_t Value,
                          const MachineRegisterInfo &MRI) const;
 
   /// Return true if the specified operand is a G_GEP with a G_CONSTANT on the
   /// right-hand side. GlobalISel's separation of pointer and integer types
   /// means that we don't need to worry about G_OR with equivalent semantics.
   bool isBaseWithConstantOffset(const MachineOperand &Root,
                                 const MachineRegisterInfo &MRI) const;

include/llvm/CodeGen/GlobalISel/InstructionSelectorImpl.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_GLOBALISEL_INSTRUCTIONSELECTORIMPL_H
 #define LLVM_CODEGEN_GLOBALISEL_INSTRUCTIONSELECTORIMPL_H
 
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/CodeGen/GlobalISel/InstructionSelector.h"
 #include "llvm/CodeGen/GlobalISel/RegisterBankInfo.h"
+#include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/CodeGen/TargetOpcodes.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/Support/Debug.h"

include/llvm/CodeGen/GlobalISel/Utils.h

 unsigned constrainOperandRegClass(const MachineFunction &MF,
                                   const TargetRegisterInfo &TRI,
                                   MachineRegisterInfo &MRI,
                                   const TargetInstrInfo &TII,
                                   const RegisterBankInfo &RBI,
                                   MachineInstr &InsertPt, const MCInstrDesc &II,
                                   unsigned Reg, unsigned OpIdx);
 
+/// Mutate the newly-selected instruction \p I to constrain its (possibly
+/// generic) virtual register operands to the instruction's register class.
+/// This could involve inserting COPYs before (for uses) or after (for defs).
+/// This requires the number of operands to match the instruction description.
+/// \returns whether operand regclass constraining succeeded.
+///
+// FIXME: Not all instructions have the same number of operands. We should
+// probably expose a constrain helper per operand and let the target selector
+// constrain individual registers, like fast-isel.
+bool constrainSelectedInstRegOperands(MachineInstr &I,
+                                      const TargetInstrInfo &TII,
+                                      const TargetRegisterInfo &TRI,
+                                      const RegisterBankInfo &RBI);
 /// Check whether an instruction \p MI is dead: it only defines dead virtual
 /// registers, and doesn't have other side effects.
 bool isTriviallyDead(const MachineInstr &MI, const MachineRegisterInfo &MRI);
 
 /// Report an ISel error as a missed optimization remark to the LLVMContext's
 /// diagnostic stream.  Set the FailedISel MachineFunction property.
 void reportGISelFailure(MachineFunction &MF, const TargetPassConfig &TPC,
                         MachineOptimizationRemarkEmitter &MORE,

include/llvm/CodeGen/MachineInstrBuilder.h

 //           .addReg(argVal2);
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CODEGEN_MACHINEINSTRBUILDER_H
 #define LLVM_CODEGEN_MACHINEINSTRBUILDER_H
 
 #include "llvm/ADT/ArrayRef.h"
+#include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBundle.h"
 #include "llvm/CodeGen/MachineOperand.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/IR/InstrTypes.h"
 #include "llvm/IR/Intrinsics.h"
public:
   }
 
   /// Copy all the implicit operands from OtherMI onto this one.
   const MachineInstrBuilder &
   copyImplicitOps(const MachineInstr &OtherMI) const {
     MI->copyImplicitOps(*MF, OtherMI);
     return *this;
   }
+
+  bool constrainAllUses(const TargetInstrInfo &TII,
+                        const TargetRegisterInfo &TRI,
+                        const RegisterBankInfo &RBI) const {
+    return constrainSelectedInstRegOperands(*MI, TII, TRI, RBI);
+  }
 };
 
 /// Builder interface. Specify how to create the initial instruction itself.
 inline MachineInstrBuilder BuildMI(MachineFunction &MF, const DebugLoc &DL,
                                    const MCInstrDesc &MCID) {
   return MachineInstrBuilder(MF, MF.CreateMachineInstr(MCID, DL));
 }
 

lib/CodeGen/GlobalISel/InstructionSelector.cpp

bool InstructionSelector::constrainOperandRegToRegClass(
   MachineBasicBlock &MBB = *I.getParent();
   MachineFunction &MF = *MBB.getParent();
   MachineRegisterInfo &MRI = MF.getRegInfo();
 
   return
       constrainRegToClass(MRI, TII, RBI, I, I.getOperand(OpIdx).getReg(), RC);
 }
 
-bool InstructionSelector::constrainSelectedInstRegOperands(
-    MachineInstr &I, const TargetInstrInfo &TII, const TargetRegisterInfo &TRI,
-    const RegisterBankInfo &RBI) const {
-  MachineBasicBlock &MBB = *I.getParent();
-  MachineFunction &MF = *MBB.getParent();
-  MachineRegisterInfo &MRI = MF.getRegInfo();
-
-  for (unsigned OpI = 0, OpE = I.getNumExplicitOperands(); OpI != OpE; ++OpI) {
-    MachineOperand &MO = I.getOperand(OpI);
-
-    // There's nothing to be done on non-register operands.
-    if (!MO.isReg())
-      continue;
-
-    DEBUG(dbgs() << "Converting operand: " << MO << '\n');
-    assert(MO.isReg() && "Unsupported non-reg operand");
-
-    unsigned Reg = MO.getReg();
-    // Physical registers don't need to be constrained.
-    if (TRI.isPhysicalRegister(Reg))
-      continue;
-
-    // Register operands with a value of 0 (e.g. predicate operands) don't need
-    // to be constrained.
-    if (Reg == 0)
-      continue;
-
-    // If the operand is a vreg, we should constrain its regclass, and only
-    // insert COPYs if that's impossible.
-    // constrainOperandRegClass does that for us.
-    MO.setReg(constrainOperandRegClass(MF, TRI, MRI, TII, RBI, I, I.getDesc(),
-                                       Reg, OpI));
-
-    // Tie uses to defs as indicated in MCInstrDesc if this hasn't already been
-    // done.
-    if (MO.isUse()) {
-      int DefIdx = I.getDesc().getOperandConstraint(OpI, MCOI::TIED_TO);
-      if (DefIdx != -1 && !I.isRegTiedToUseOperand(DefIdx))
-        I.tieOperands(DefIdx, OpI);
-    }
-  }
-  return true;
-}
-
 bool InstructionSelector::isOperandImmEqual(
     const MachineOperand &MO, int64_t Value,
     const MachineRegisterInfo &MRI) const {
   if (MO.isReg() && MO.getReg())
     if (auto VRegVal = getConstantVRegVal(MO.getReg(), MRI))
       return *VRegVal == Value;
   return false;
 }

lib/CodeGen/GlobalISel/Utils.cpp

unsigned llvm::constrainOperandRegClass(
   // Assume physical registers are properly constrained.
   assert(TargetRegisterInfo::isVirtualRegister(Reg) &&
          "PhysReg not implemented");
 
   const TargetRegisterClass *RegClass = TII.getRegClass(II, OpIdx, &TRI, MF);
   return constrainRegToClass(MRI, TII, RBI, InsertPt, Reg, *RegClass);
 }
 
+bool llvm::constrainSelectedInstRegOperands(MachineInstr &I,
+                                            const TargetInstrInfo &TII,
+                                            const TargetRegisterInfo &TRI,
+                                            const RegisterBankInfo &RBI) {
+  MachineBasicBlock &MBB = *I.getParent();
+  MachineFunction &MF = *MBB.getParent();
+  MachineRegisterInfo &MRI = MF.getRegInfo();
+
+  for (unsigned OpI = 0, OpE = I.getNumExplicitOperands(); OpI != OpE; ++OpI) {
+    MachineOperand &MO = I.getOperand(OpI);
+
+    // There's nothing to be done on non-register operands.
+    if (!MO.isReg())
+      continue;
+
+    DEBUG(dbgs() << "Converting operand: " << MO << '\n');
+    assert(MO.isReg() && "Unsupported non-reg operand");
+
+    unsigned Reg = MO.getReg();
+    // Physical registers don't need to be constrained.
+    if (TRI.isPhysicalRegister(Reg))
+      continue;
+
+    // Register operands with a value of 0 (e.g. predicate operands) don't need
+    // to be constrained.
+    if (Reg == 0)
+      continue;
+
+    // If the operand is a vreg, we should constrain its regclass, and only
+    // insert COPYs if that's impossible.
+    // constrainOperandRegClass does that for us.
+    MO.setReg(constrainOperandRegClass(MF, TRI, MRI, TII, RBI, I, I.getDesc(),
+                                       Reg, OpI));
+
+    // Tie uses to defs as indicated in MCInstrDesc if this hasn't already been
+    // done.
+    if (MO.isUse()) {
+      int DefIdx = I.getDesc().getOperandConstraint(OpI, MCOI::TIED_TO);
+      if (DefIdx != -1 && !I.isRegTiedToUseOperand(DefIdx))
+        I.tieOperands(DefIdx, OpI);
+    }
+  }
+  return true;
+}
+
 bool llvm::isTriviallyDead(const MachineInstr &MI,
                            const MachineRegisterInfo &MRI) {
   // If we can move an instruction, we can remove it.  Otherwise, it has
   // a side-effect of some sort.
   bool SawStore = false;
   if (!MI.isSafeToMove(/*AA=*/nullptr, SawStore))
     return false;
 

lib/Target/AArch64/AArch64InstructionSelector.cpp

bool AArch64InstructionSelector::selectCompareBranch(
   unsigned CBOpc = 0;
   if (CmpWidth <= 32)
     CBOpc = (Pred == CmpInst::ICMP_EQ ? AArch64::CBZW : AArch64::CBNZW);
   else if (CmpWidth == 64)
     CBOpc = (Pred == CmpInst::ICMP_EQ ? AArch64::CBZX : AArch64::CBNZX);
   else
     return false;
 
-  auto MIB = BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(CBOpc))
-                 .addUse(LHS)
-                 .addMBB(DestMBB);
+  BuildMI(*I.getParent(), I, I.getDebugLoc(), TII.get(CBOpc))
+      .addMBB(DestMBB)
+      .constrainAllUses(TII, TRI, RBI);
 
-  constrainSelectedInstRegOperands(*MIB.getInstr(), TII, TRI, RBI);
   I.eraseFromParent();
   return true;
 }
 
 bool AArch64InstructionSelector::selectVaStartAAPCS(
     MachineInstr &I, MachineFunction &MF, MachineRegisterInfo &MRI) const {
   return false;
 }

lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

 /// \todo This should be generated by TableGen.
 //===----------------------------------------------------------------------===//
 
 #include "AMDGPUInstructionSelector.h"
 #include "AMDGPUInstrInfo.h"
 #include "AMDGPURegisterBankInfo.h"
 #include "AMDGPURegisterInfo.h"
 #include "AMDGPUSubtarget.h"
+#include "llvm/CodeGen/GlobalISel/Utils.h"
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineInstr.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/IR/Type.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"