[GlobalISel] improved interface to MachineIRBuilder::buildInstr

[t.p.northover]

Summary

Hi,

I've been thinking about how to make MachineInstrBuilder scale as pleasantly as possible, and come up with this idea. Basically, instead of

MIRBuilder.buildInstr(Opcode, Ty, Res, Op0, Op1);

you'd have to write

MIRBuilder.buildInstr(Opcode, Ty, MIB::Def(Res), MIB::Use(Op0), MIB::Use(Op1));

(and similar). This has a few advantages:

• Removes the implicit specialness of the first register (inherited from BuildMI, where it's caught out at least two people I know of, including me).
• Allows other kinds of operands that we'll want to be handled uniformly (FrameIndex, MBB, Imm, ...).

On the other hand, there are disadvantages:

• The usual opaque template error messages when you get it wrong.
• More verbose in the common case of single-def, multiple-uses, register only (instructions like this will be less common for target opcodes than generic ones though).
• Feels like a minor riff on the old MachineInstrBuilder interface. Perhaps what we'd have used then if we had variadic templates, or perhaps there's a good reason we didn't?

Anyone got any feelings? On the whole I think it has potential.

[qcolombet]

Hi Tim,

I like the idea!
That's a +1 for me!

Cheers,
Q.

[arsenm]

This seems less awkward for instructions with multiple results, adding a separate addReg(x, RegState::Define) always feels dirty

[t.p.northover]

OK, given the initial responses I've polished the patch up a bit and I'm converting it into a real review. I've not added all the weird corner-cases yet since we've got no real way to use them. Extending the interface as GlobalISel gains the features needed to test them ought to be a simple copy/paste/edit job.

OK to commit as is, or does anyone have some suggestions for improvements?

[ab]

It does seem nicer!

Here's a simpler alternative: have buildInstr return the builder, and replace addReg with explicit addDef/addUse? That spares you the template hackery for a similar result. WDYT?

[t.p.northover]

Excellent idea! I can't really see any aspect it doesn't improve on this patch. I'll get to work.

[t.p.northover]

Simplify the change and interface based on Ahmed's idea.

[• tstellarAMD]

In D22820#497728, @ab wrote:

It does seem nicer!

Here's a simpler alternative: have buildInstr return the builder, and replace addReg with explicit addDef/addUse? That spares you the template hackery for a similar result. WDYT?

I also like this idea, because it makes it really easy to add immediate operands.

[qcolombet]

This landed in r277176 as far as I can tell.

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

 
 #include "llvm/CodeGen/MachineBasicBlock.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/LowLevelType.h"
 #include "llvm/IR/DebugLoc.h"
 
 namespace llvm {
 
+namespace MIB {
+/// Wrapper for a VReg use or def to be used with `MachineIRBuilder::buildInstr`
+/// variants. If you know whether the register will be used or defined at
+/// compile-time, consider using `MIB::Use` or `MIB::Def` instead.
+struct Reg {
+  unsigned TheReg;
+  unsigned Flags;
+
+  explicit Reg(unsigned TheReg, unsigned Flags = 0)
+      : TheReg(TheReg), Flags(Flags) {}
+};
+
+// Two helper classes to explicitly create statically-known uses and defs in a
+// uniform manner. They are implemented as classes rather than functions to
+// allow the stricter type-checking provided by braced initialization.
+//
+// Beware, during use these are converted back to a Reg value, with all the
+// slicing possibilities C++ offers: make sure they never develop any data
+// members you care about.
+
+/// Create a VReg definition to be used with `MachineIRBuilder::buildInstr`
+struct Def : Reg {
+  Def(unsigned TheReg, unsigned ExtraFlags = 0)
+      : Reg{TheReg, ExtraFlags | RegState::Define} {}
+};
+
+/// Create a VReg use to be used with `MachineIRBuilder::buildInstr`
+struct Use : Reg {
+  Use(unsigned TheReg, unsigned Flags = 0) : Reg{TheReg, Flags} {
+    assert(!(Flags & RegState::Define) && "misleading register definition");
+  }
+};
+
+/// Wrapper for frame index to be used with `MachineIRBuilder::buildInstr`
+struct FI {
+  int Idx;
+  explicit FI(int Idx) : Idx(Idx) {}
+};
+
+/// Wrapper for immediate to be used with `MachineIRBuilder::buildInstr`
+struct Imm {
+  int64_t Val;
+  explicit Imm(int64_t Val) : Val(Val) {}
+};
+
+/// Wrapper for MachineBasicBlock to be used with `MachineIRBuilder::buildInstr`
+struct MBB {
+  MachineBasicBlock &BB;
+  explicit MBB(MachineBasicBlock &BB) : BB(BB) {}
+};
+
+}
+
 // Forward declarations.
 class MachineFunction;
 class MachineInstr;
 class TargetInstrInfo;
 
 /// Helper class to build MachineInstr.
 /// It keeps internally the insertion point and debug location for all
 /// the new instructions we want to create.
class MachineIRBuilder {
   bool Before;
   /// @}
 
   const TargetInstrInfo &getTII() {
     assert(TII && "TargetInstrInfo is not set");
     return *TII;
   }
 
-  static void addRegs(MachineInstrBuilder &MIB) {}
+  static void addOperands(MachineInstrBuilder &MIB) {}
+
+  template <typename... MoreOps>
+  static void addOperands(MachineInstrBuilder &MIB, MIB::Reg Reg,
+                          MoreOps ... Ops) {
+    MIB.addReg(Reg.TheReg, Reg.Flags);
+    addOperands(MIB, Ops...);
+  }
+
+  template <typename... MoreOps>
+  static void addOperands(MachineInstrBuilder &MIB, MIB::Imm Imm,
+                          MoreOps ... Ops) {
+    MIB.addImm(Imm.Val);
+    addOperands(MIB, Ops...);
+  }
 
-  template <typename... MoreRegs>
-  static void addRegs(MachineInstrBuilder &MIB, unsigned Reg,
-                      MoreRegs... Regs) {
-    MIB.addReg(Reg);
-    addRegs(MIB, Regs...);
+  template <typename... MoreOps>
+  static void addOperands(MachineInstrBuilder &MIB, MIB::FI Idx,
+                          MoreOps... Ops) {
+    MIB.addFrameIndex(Idx.Idx);
+    addOperands(MIB, Ops...);
   }
 
+  template <typename... MoreOps>
+  static void addOperands(MachineInstrBuilder &MIB, MIB::MBB MBB,
+                          MoreOps... Ops) {
+    MIB.addMBB(&MBB.BB);
+    addOperands(MIB, Ops...);
+  }
 
 public:
   /// Getter for the function we currently build.
   MachineFunction &getMF() {
     assert(MF && "MachineFunction is not set");
     return *MF;
   }
 
public:
   /// \pre MI must be in getMF().
   void setInstr(MachineInstr &MI, bool Before = true);
   /// @}
 
   /// Set the debug location to \p DL for all the next build instructions.
   void setDebugLoc(const DebugLoc &DL) { this->DL = DL; }
 
   /// Build and insert <empty> = \p Opcode [ { \p Tys } ] <empty>.
-  /// \p Ty is the type of the instruction if \p Opcode describes
-  /// a generic machine instruction. \p Ty must be LLT{} if \p Opcode
+  /// \p Tys is a list of the types of the instruction if \p Opcode describes
+  /// a generic machine instruction. \p Tys must be empty if \p Opcode
   /// does not describe a generic instruction.
   /// The insertion point is the one set by the last call of either
   /// setBasicBlock or setMI.
   ///
   /// \pre setBasicBlock or setMI must have been called.
-  /// \pre Ty == LLT{} or isPreISelGenericOpcode(Opcode)
+  /// \pre Tys.empty() or isPreISelGenericOpcode(Opcode)
   ///
   /// \return The newly created instruction.
   MachineInstr *buildInstr(unsigned Opcode, ArrayRef<LLT> Tys);
 
-  /// Build and insert \p Res = \p Opcode [\p Ty] \p Uses....
-  /// \p Ty is the type of the instruction if \p Opcode describes
-  /// a generic machine instruction. \p Ty must be LLT{} if \p Opcode
-  /// does not describe a generic instruction.
-  /// The insertion point is the one set by the last call of either
-  /// setBasicBlock or setMI.
+  /// Build and insert `Res(s)... = Opcode [ Ty] Use(s)...`. \p Ty is the
+  /// type of the instruction if \p Opcode describes a generic machine
+  /// instruction. \p Ty must be LLT{} if \p Opcode does not describe a generic
+  /// instruction.  The insertion point is the one set by the last call of
+  /// either setBasicBlock or setMI.
   ///
   /// \pre setBasicBlock or setMI must have been called.
-  /// \pre Ty == LLT{} or isPreISelGenericOpcode(Opcode)
+  /// \pre Tys.empty() or isPreISelGenericOpcode(Opcode)
+  /// \pre Each \p Ops parameter must be an instance of a class in the namespace
+  /// llvm::MIB.
   ///
   /// \return The newly created instruction.
-  template <typename... MoreRegs>
-  MachineInstr *buildInstr(unsigned Opcode, ArrayRef<LLT> Tys, unsigned Res,
-                           MoreRegs... Uses) {
+  template <typename... MoreOps>
+  MachineInstr *buildInstr(unsigned Opcode, ArrayRef<LLT> Tys,
+                           MoreOps... Ops) {
     MachineInstr *NewMI = buildInstr(Opcode, Tys);
     MachineInstrBuilder MIB{getMF(), NewMI};
-    MIB.addReg(Res, RegState::Define);
-    addRegs(MIB, Uses...);
-
+    addOperands(MIB, Ops...);
     return NewMI;
   }
 
-  /// Build and insert <empty> = \p Opcode <empty>.
+  /// Build and insert `Res(s)... = Opcode Use(s)...`. The insertion point is
+  /// the one set by the last call of either setBasicBlock or setMI.
   ///
   /// \pre setBasicBlock or setMI must have been called.
-  /// \pre not isPreISelGenericOpcode(\p Opcode)
+  /// \pre Each \p Ops parameter must be an instance of a class in the namespace
+  /// llvm::MIB.
   ///
   /// \return The newly created instruction.
-  MachineInstr *buildInstr(unsigned Opcode) {
-    return buildInstr(Opcode, ArrayRef<LLT>());
+  template <typename... MoreOps>
+  MachineInstr *buildInstr(unsigned Opcode, MoreOps... Ops) {
+    return buildInstr(Opcode, ArrayRef<LLT>(), Ops...);
   }
 
-  /// Build and insert \p Res = \p Opcode \p Uses....
-  /// The insertion point is the one set by the last call of either
-  /// setBasicBlock or setMI.
-  ///
-  /// \pre setBasicBlock or setMI must have been called.
-  ///
-  /// \return The newly created instruction.
-  template <typename... MoreRegs>
-  MachineInstr *buildInstr(unsigned Opcode, unsigned Res, MoreRegs... Uses) {
-    return buildInstr(Opcode, ArrayRef<LLT>(), Res, Uses...);
+  // This version is needed because otherwise the buildInstr(unsigned,
+  // MoreOps...) instance is preferred, and then instantiation fails when there
+  // is no specialization available for buildInstr(unsigned, ArrayRef<LLT>,
+  // Ops...)  where Ops contains an LLT.
+  template <typename... MoreOps>
+  MachineInstr *buildInstr(unsigned Opcode, LLT Ty,
+                           MoreOps... Ops) {
+    assert(isPreISelGenericOpcode(Opcode) &&
+           "unexpected type on non-generic opcode");
+    return buildInstr(Opcode, ArrayRef<LLT>(Ty), Ops...);
   }
 
   /// Build and insert \p Res<def> = G_FRAME_INDEX \p Ty \p Idx
   ///
   /// G_FRAME_INDEX materializes the address of an alloca value or other
   /// stack-based object.
   ///
   /// \pre setBasicBlock or setMI must have been called.

lib/CodeGen/GlobalISel/IRTranslator.cpp

 bool IRTranslator::translateBinaryOp(unsigned Opcode, const Instruction &Inst) {
   // Get or create a virtual register for each value.
   // Unless the value is a Constant => loadimm cst?
   // or inline constant each time?
   // Creation of a virtual register needs to have a size.
   unsigned Op0 = getOrCreateVReg(*Inst.getOperand(0));
   unsigned Op1 = getOrCreateVReg(*Inst.getOperand(1));
   unsigned Res = getOrCreateVReg(Inst);
-  MIRBuilder.buildInstr(Opcode, LLT{*Inst.getType()}, Res, Op0, Op1);
+  MIRBuilder.buildInstr(Opcode, LLT{*Inst.getType()}, MIB::Def(Res),
+                        MIB::Use(Op0), MIB::Use(Op1));
   return true;
 }
 
 bool IRTranslator::translateReturn(const Instruction &Inst) {
   assert(isa<ReturnInst>(Inst) && "Return expected");
   const Value *Ret = cast<ReturnInst>(Inst).getReturnValue();
   // The target may mess up with the insertion point, but
   // this is not important as a return is the last instruction
bool IRTranslator::translateBitCast(const CastInst &CI) {
   }
   return translateCast(TargetOpcode::G_BITCAST, CI);
 }
 
 bool IRTranslator::translateCast(unsigned Opcode, const CastInst &CI) {
   unsigned Op = getOrCreateVReg(*CI.getOperand(0));
   unsigned Res = getOrCreateVReg(CI);
   MIRBuilder.buildInstr(Opcode, {LLT{*CI.getDestTy()}, LLT{*CI.getSrcTy()}},
-                        Res, Op);
+                        MIB::Def(Res), MIB::Use(Op));
   return true;
 }
 
 bool IRTranslator::translateStaticAlloca(const AllocaInst &AI) {
   assert(AI.isStaticAlloca() && "only handle static allocas now");
   MachineFunction &MF = MIRBuilder.getMF();
   unsigned ElementSize = DL->getTypeStoreSize(AI.getAllocatedType());
   unsigned Size =

lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

MachineInstr *MachineIRBuilder::buildInstr(unsigned Opcode, ArrayRef<LLT> Tys) {
   } else
     assert(!isPreISelGenericOpcode(Opcode) &&
            "Generic instruction must have a type");
   getMBB().insert(getInsertPt(), NewMI);
   return NewMI;
 }
 
 MachineInstr *MachineIRBuilder::buildFrameIndex(LLT Ty, unsigned Res, int Idx) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_FRAME_INDEX, Ty);
-  auto MIB = MachineInstrBuilder(getMF(), NewMI);
-  MIB.addReg(Res, RegState::Define);
-  MIB.addFrameIndex(Idx);
-  return NewMI;
+  return buildInstr(TargetOpcode::G_FRAME_INDEX, Ty, MIB::Def{Res},
+                    MIB::FI{Idx});
 }
 
 MachineInstr *MachineIRBuilder::buildAdd(LLT Ty, unsigned Res, unsigned Op0,
                                          unsigned Op1) {
-  return buildInstr(TargetOpcode::G_ADD, Ty, Res, Op0, Op1);
+  return buildInstr(TargetOpcode::G_ADD, Ty, MIB::Def{Res}, MIB::Use{Op0},
+                    MIB::Use{Op1});
 }
 
 MachineInstr *MachineIRBuilder::buildBr(MachineBasicBlock &Dest) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_BR, LLT::unsized());
-  MachineInstrBuilder(getMF(), NewMI).addMBB(&Dest);
-  return NewMI;
+  return buildInstr(TargetOpcode::G_BR, LLT::unsized(), MIB::MBB{Dest});
 }
 
 MachineInstr *MachineIRBuilder::buildCopy(unsigned Res, unsigned Op) {
-  return buildInstr(TargetOpcode::COPY, Res, Op);
+  return buildInstr(TargetOpcode::COPY, MIB::Def{Res}, MIB::Use{Op});
 }
 
 MachineInstr *MachineIRBuilder::buildLoad(LLT VTy, LLT PTy, unsigned Res,
                                           unsigned Addr,
                                           MachineMemOperand &MMO) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_LOAD, {VTy, PTy}, Res, Addr);
+  auto NewMI = buildInstr(TargetOpcode::G_LOAD, {VTy, PTy}, MIB::Def{Res},
+                          MIB::Use{Addr});
   NewMI->addMemOperand(getMF(), &MMO);
   return NewMI;
 }
 
 MachineInstr *MachineIRBuilder::buildStore(LLT VTy, LLT PTy, unsigned Val,
                                            unsigned Addr,
                                            MachineMemOperand &MMO) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_STORE, {VTy, PTy});
+  auto NewMI = buildInstr(TargetOpcode::G_STORE, {VTy, PTy}, MIB::Use{Val},
+                          MIB::Use{Addr});
   NewMI->addMemOperand(getMF(), &MMO);
-  MachineInstrBuilder(getMF(), NewMI).addReg(Val).addReg(Addr);
   return NewMI;
 }
 
 MachineInstr *MachineIRBuilder::buildExtract(LLT Ty, ArrayRef<unsigned> Results,
                                              unsigned Src,
                                              ArrayRef<unsigned> Indexes) {
   assert(Results.size() == Indexes.size() && "inconsistent number of regs");
 

lib/Target/AArch64/AArch64CallLowering.cpp

assert((Val->getType()->isIntegerTy() || Val->getType()->isPointerTy()) &&
            "Type not supported yet");
     const Function &F = *MIRBuilder.getMF().getFunction();
     const DataLayout &DL = F.getParent()->getDataLayout();
     unsigned Size = DL.getTypeSizeInBits(Val->getType());
     assert((Size == 64 || Size == 32) && "Size not supported yet");
     unsigned ResReg = (Size == 32) ? AArch64::W0 : AArch64::X0;
     // Set the insertion point to be right before Return.
     MIRBuilder.setInstr(*Return, /* Before */ true);
-    MachineInstr *Copy =
-        MIRBuilder.buildInstr(TargetOpcode::COPY, ResReg, VReg);
+    MachineInstr *Copy = MIRBuilder.buildCopy(ResReg, VReg);
     (void)Copy;
     assert(Copy->getNextNode() == Return &&
            "The insertion did not happen where we expected");
     MachineInstrBuilder(MIRBuilder.getMF(), Return)
         .addReg(ResReg, RegState::Implicit);
   }
   return true;
 }
bool AArch64CallLowering::lowerFormalArguments(
   assert(ArgLocs.size() == Args.size() &&
          "We have a different number of location and args?!");
   for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
     CCValAssign &VA = ArgLocs[i];
 
     assert(VA.isRegLoc() && "Not yet implemented");
     // Transform the arguments in physical registers into virtual ones.
     MIRBuilder.getMBB().addLiveIn(VA.getLocReg());
-    MIRBuilder.buildInstr(TargetOpcode::COPY, VRegs[i], VA.getLocReg());
+    MIRBuilder.buildCopy(VRegs[i], VA.getLocReg());
 
     switch (VA.getLocInfo()) {
     default:
       llvm_unreachable("Unknown loc info!");
     case CCValAssign::Full:
       break;
     case CCValAssign::BCvt:
       // We don't care about bitcast.