[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

class MachineIRBuilder {
   bool Before;
   /// @}
 
   const TargetInstrInfo &getTII() {
     assert(TII && "TargetInstrInfo is not set");
     return *TII;
   }
 
-  static void addRegs(MachineInstrBuilder &MIB) {}
-
-  template <typename... MoreRegs>
-  static void addRegs(MachineInstrBuilder &MIB, unsigned Reg,
-                      MoreRegs... Regs) {
-    MIB.addReg(Reg);
-    addRegs(MIB, Regs...);
-  }
-
-
 public:
   /// Getter for the function we currently build.
   MachineFunction &getMF() {
     assert(MF && "MachineFunction is not set");
     return *MF;
   }
 
   /// Getter for the basic block we currently build.
public:
   /// 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)
   ///
-  /// \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.
-  ///
-  /// \pre setBasicBlock or setMI must have been called.
-  /// \pre Ty == LLT{} or isPreISelGenericOpcode(Opcode)
-  ///
-  /// \return The newly created instruction.
-  template <typename... MoreRegs>
-  MachineInstr *buildInstr(unsigned Opcode, ArrayRef<LLT> Tys, unsigned Res,
-                           MoreRegs... Uses) {
-    MachineInstr *NewMI = buildInstr(Opcode, Tys);
-    MachineInstrBuilder MIB{getMF(), NewMI};
-    MIB.addReg(Res, RegState::Define);
-    addRegs(MIB, Uses...);
-
-    return NewMI;
-  }
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildInstr(unsigned Opcode, ArrayRef<LLT> Tys);
 
   /// Build and insert <empty> = \p Opcode <empty>.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre not isPreISelGenericOpcode(\p Opcode)
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildInstr(unsigned Opcode) {
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildInstr(unsigned Opcode) {
     return buildInstr(Opcode, ArrayRef<LLT>());
   }
 
-  /// 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...);
-  }
-
   /// 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.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildFrameIndex(LLT Ty, unsigned Res, int Idx);
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildFrameIndex(LLT Ty, unsigned Res, int Idx);
 
   /// Build and insert \p Res<def> = G_ADD \p Ty \p Op0, \p Op1
   ///
   /// G_ADD sets \p Res to the sum of integer parameters \p Op0 and \p Op1,
   /// truncated to their width.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildAdd(LLT Ty, unsigned Res, unsigned Op0, unsigned Op1);
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildAdd(LLT Ty, unsigned Res, unsigned Op0,
+                                unsigned Op1);
 
   /// Build and insert G_BR unsized \p Dest
   ///
   /// G_BR is an unconditional branch to \p Dest.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildBr(MachineBasicBlock &BB);
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildBr(MachineBasicBlock &BB);
 
   /// Build and insert \p Res<def> = COPY Op
   ///
   /// Register-to-register COPY sets \p Res to \p Op.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildCopy(unsigned Res, unsigned Op);
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildCopy(unsigned Res, unsigned Op);
 
   /// Build and insert `Res<def> = G_LOAD { VTy, PTy } Addr, MMO`.
   ///
   /// Loads the value of (sized) type \p VTy stored at \p Addr (in address space
   /// given by \p PTy). Puts the result in Res.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildLoad(LLT VTy, LLT PTy, unsigned Res, unsigned Addr,
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildLoad(LLT VTy, LLT PTy, unsigned Res, unsigned Addr,
                                 MachineMemOperand &MMO);
 
   /// Build and insert `G_STORE { VTy, PTy } Val, Addr, MMO`.
   ///
   /// Stores the value \p Val of (sized) \p VTy to \p Addr (in address space
   /// given by \p PTy).
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildStore(LLT VTy, LLT PTy, unsigned Val, unsigned Addr,
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildStore(LLT VTy, LLT PTy, unsigned Val, unsigned Addr,
                                  MachineMemOperand &MMO);
 
   /// Build and insert `Res0<def>, ... = G_EXTRACT Ty Src, Idx0, ...`.
   ///
   /// If \p Ty has size N bits, G_EXTRACT sets \p Res[0] to bits `[Idxs[0],
   /// Idxs[0] + N)` of \p Src and similarly for subsequent bit-indexes.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildExtract(LLT Ty, ArrayRef<unsigned> Results, unsigned Src,
-                             ArrayRef<unsigned> Indexes);
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildExtract(LLT Ty, ArrayRef<unsigned> Results,
+                                   unsigned Src, ArrayRef<unsigned> Indexes);
 
   /// Build and insert \p Res<def> = G_SEQUENCE \p Ty \p Ops[0], ...
   ///
   /// G_SEQUENCE concatenates each element in Ops into a single register, where
   /// Ops[0] starts at bit 0 of \p Res.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre The sum of the input sizes must equal the result's size.
   ///
-  /// \return The newly created instruction.
-  MachineInstr *buildSequence(LLT Ty, unsigned Res, ArrayRef<unsigned> Ops);
+  /// \return a MachineInstrBuilder for the newly created instruction.
+  MachineInstrBuilder buildSequence(LLT Ty, unsigned Res,
+                                    ArrayRef<unsigned> Ops);
 };
 
 } // End namespace llvm.
 #endif // LLVM_CODEGEN_GLOBALISEL_MACHINEIRBUILDER_H

include/llvm/CodeGen/MachineInstrBuilder.h

MI->addOperand(*MF, MachineOperand::CreateReg(RegNo,
                                                flags & RegState::Undef,
                                                flags & RegState::EarlyClobber,
                                                SubReg,
                                                flags & RegState::Debug,
                                                flags & RegState::InternalRead));
     return *this;
   }
 
+  /// Add a virtual register definition operand.
+  const MachineInstrBuilder &addDef(unsigned RegNo, unsigned Flags = 0,
+                                    unsigned SubReg = 0) const {
+    return addReg(RegNo, Flags | RegState::Define, SubReg);
+  }
+
+  /// Add a virtual register use operand. It is an error for Flags to contain
+  /// `RegState::Define` when calling this function.
+  const MachineInstrBuilder &addUse(unsigned RegNo, unsigned Flags = 0,
+                                    unsigned SubReg = 0) const {
+    assert(!(Flags & RegState::Define) &&
+           "Misleading addUse defines register, use addReg instead.");
+    return addReg(RegNo, Flags, SubReg);
+  }
+
   /// Add a new immediate operand.
   const MachineInstrBuilder &addImm(int64_t Val) const {
     MI->addOperand(*MF, MachineOperand::CreateImm(Val));
     return *this;
   }
 
   const MachineInstrBuilder &addCImm(const ConstantInt *Val) const {
     MI->addOperand(*MF, MachineOperand::CreateCImm(Val));

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()})
+      .addDef(Res)
+      .addUse(Op0)
+      .addUse(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
if (LLT{*CI.getDestTy()} == LLT{*CI.getSrcTy()}) {
     return true;
   }
   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);
+  MIRBuilder.buildInstr(Opcode, {LLT{*CI.getDestTy()}, LLT{*CI.getSrcTy()}})
+      .addDef(Res)
+      .addUse(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

MachineBasicBlock::iterator MachineIRBuilder::getInsertPt() {
   }
   return Before ? getMBB().begin() : getMBB().end();
 }
 
 //------------------------------------------------------------------------------
 // Build instruction variants.
 //------------------------------------------------------------------------------
 
-MachineInstr *MachineIRBuilder::buildInstr(unsigned Opcode, ArrayRef<LLT> Tys) {
-  MachineInstr *NewMI = BuildMI(getMF(), DL, getTII().get(Opcode));
+MachineInstrBuilder MachineIRBuilder::buildInstr(unsigned Opcode,
+                                                  ArrayRef<LLT> Tys) {
+  MachineInstrBuilder MIB = BuildMI(getMF(), DL, getTII().get(Opcode));
   if (Tys.size() > 0) {
     assert(isPreISelGenericOpcode(Opcode) &&
            "Only generic instruction can have a type");
     for (unsigned i = 0; i < Tys.size(); ++i)
-      NewMI->setType(Tys[i], i);
+      MIB->setType(Tys[i], i);
   } else
     assert(!isPreISelGenericOpcode(Opcode) &&
            "Generic instruction must have a type");
-  getMBB().insert(getInsertPt(), NewMI);
-  return NewMI;
+  getMBB().insert(getInsertPt(), MIB);
+  return MIB;
 }
 
-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;
+MachineInstrBuilder MachineIRBuilder::buildFrameIndex(LLT Ty, unsigned Res,
+                                                       int Idx) {
+  return buildInstr(TargetOpcode::G_FRAME_INDEX, Ty)
+      .addDef(Res)
+      .addFrameIndex(Idx);
 }
 
-MachineInstr *MachineIRBuilder::buildAdd(LLT Ty, unsigned Res, unsigned Op0,
-                                         unsigned Op1) {
-  return buildInstr(TargetOpcode::G_ADD, Ty, Res, Op0, Op1);
+MachineInstrBuilder MachineIRBuilder::buildAdd(LLT Ty, unsigned Res,
+                                                unsigned Op0, unsigned Op1) {
+  return buildInstr(TargetOpcode::G_ADD, Ty)
+      .addDef(Res)
+      .addUse(Op0)
+      .addUse(Op1);
 }
 
-MachineInstr *MachineIRBuilder::buildBr(MachineBasicBlock &Dest) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_BR, LLT::unsized());
-  MachineInstrBuilder(getMF(), NewMI).addMBB(&Dest);
-  return NewMI;
+MachineInstrBuilder MachineIRBuilder::buildBr(MachineBasicBlock &Dest) {
+  return buildInstr(TargetOpcode::G_BR, LLT::unsized()).addMBB(&Dest);
 }
 
-MachineInstr *MachineIRBuilder::buildCopy(unsigned Res, unsigned Op) {
-  return buildInstr(TargetOpcode::COPY, Res, Op);
+MachineInstrBuilder MachineIRBuilder::buildCopy(unsigned Res, unsigned Op) {
+  return buildInstr(TargetOpcode::COPY).addDef(Res).addUse(Op);
 }
 
-MachineInstr *MachineIRBuilder::buildLoad(LLT VTy, LLT PTy, unsigned Res,
+MachineInstrBuilder MachineIRBuilder::buildLoad(LLT VTy, LLT PTy, unsigned Res,
                                                  unsigned Addr,
                                                  MachineMemOperand &MMO) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_LOAD, {VTy, PTy}, Res, Addr);
-  NewMI->addMemOperand(getMF(), &MMO);
-  return NewMI;
+  return buildInstr(TargetOpcode::G_LOAD, {VTy, PTy})
+      .addDef(Res)
+      .addUse(Addr)
+      .addMemOperand(&MMO);
 }
 
-MachineInstr *MachineIRBuilder::buildStore(LLT VTy, LLT PTy, unsigned Val,
-                                           unsigned Addr,
+MachineInstrBuilder MachineIRBuilder::buildStore(LLT VTy, LLT PTy,
+                                                  unsigned Val, unsigned Addr,
                                                   MachineMemOperand &MMO) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_STORE, {VTy, PTy});
-  NewMI->addMemOperand(getMF(), &MMO);
-  MachineInstrBuilder(getMF(), NewMI).addReg(Val).addReg(Addr);
-  return NewMI;
+  return buildInstr(TargetOpcode::G_STORE, {VTy, PTy})
+      .addUse(Val)
+      .addUse(Addr)
+      .addMemOperand(&MMO);
 }
 
-MachineInstr *MachineIRBuilder::buildExtract(LLT Ty, ArrayRef<unsigned> Results,
-                                             unsigned Src,
-                                             ArrayRef<unsigned> Indexes) {
+MachineInstrBuilder
+MachineIRBuilder::buildExtract(LLT Ty, ArrayRef<unsigned> Results, unsigned Src,
   assert(Results.size() == Indexes.size() && "inconsistent number of regs");
 
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_EXTRACT, Ty);
-  auto MIB = MachineInstrBuilder(getMF(), NewMI);
+  MachineInstrBuilder MIB = buildInstr(TargetOpcode::G_EXTRACT, Ty);
   for (auto Res : Results)
-    MIB.addReg(Res, RegState::Define);
+    MIB.addDef(Res);
 
-  MIB.addReg(Src);
+  MIB.addUse(Src);
 
   for (auto Idx : Indexes)
     MIB.addImm(Idx);
-  return NewMI;
+  return MIB;
 }
 
-MachineInstr *MachineIRBuilder::buildSequence(LLT Ty, unsigned Res,
+MachineInstrBuilder MachineIRBuilder::buildSequence(LLT Ty, unsigned Res,
                                               ArrayRef<unsigned> Ops) {
-  MachineInstr *NewMI = buildInstr(TargetOpcode::G_SEQUENCE, Ty);
-  auto MIB = MachineInstrBuilder(getMF(), NewMI);
-  MIB.addReg(Res, RegState::Define);
+  MachineInstrBuilder MIB = buildInstr(TargetOpcode::G_SEQUENCE, Ty);
+  MIB.addDef(Res);
   for (auto Op : Ops)
-    MIB.addReg(Op);
-  return NewMI;
+    MIB.addUse(Op);
+  return MIB;
 }

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.