[GISel]: Split buildSources part of buildInstr separately

[aditya_nandakumar]

Summary

For cases where there are no arguments (creating IMPLICIT_DEF), don't attempt to create zero sized arrays.

[dsanders]

I'm not sure I understand why this change is needed. I'd expect the original version of buildInstr<unsigned>, to expand to:

MachineInstrBuilder buildInstr(unsigned Opc, unsigned &&Ty) {
  auto MIB = buildInstr(Opc).addDef(getDestFromArg(Ty));
  unsigned It[] = {};
  for (const auto &i : It)
    MIB.addUse(i);
  return MIB;
}

which should then optimize to:

MachineInstrBuilder buildInstr(unsigned Opc, unsigned &&Ty) {
  auto MIB = buildInstr(Opc).addDef(getDestFromArg(Ty));
  return MIB;
}

which appears to be the same thing the new version expands and optimizes to.

Am I missing something?

[aditya_nandakumar]

Add the ability to accept machine operands to the buildInstr method so it will directly call MIB.add(MO) on that.

[aditya_nandakumar]

@dsanders - the main goal was to avoid the compiler warning of having to create a zero sized array. Now with the splitting of arguments, we can also accept MachineOperands which can directly be added to the instruction.

[dsanders]

In D35516#819039, @aditya_nandakumar wrote:

@dsanders - the main goal was to avoid the compiler warning of having to create a zero sized array.

You mean the unsigned It[] = {};? Ok, that makes sense. The subject line should reflect that though. E.g:

[GISel] Avoid zero-length array when adding uses to instructions that lack them (e.g IMPLICIT_DEF).

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

77–80

This should be added by the patch that uses it.

81

Just for consistency, should we name this addUsesFromArgs()?

84–85

I think this is missing some std::forward's

[aditya_nandakumar]

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

77–80

Will remove this part. Will add it later.

[aditya_nandakumar]

Updated based on feedback.

[aditya_nandakumar]

Ping?

[dsanders]

LGTM with a better commit message.

[aditya_nandakumar]

Thanks. Closing.

include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

class MachineIRBuilder {
   unsigned getDestFromArg(unsigned Reg) { return Reg; }
   unsigned getDestFromArg(LLT Ty) {
     return getMF().getRegInfo().createGenericVirtualRegister(Ty);
   }
   unsigned getDestFromArg(const TargetRegisterClass *RC) {
     return getMF().getRegInfo().createVirtualRegister(RC);
   }
 
-  unsigned getRegFromArg(unsigned Reg) { return Reg; }
+  void addUseFromArg(MachineInstrBuilder &MIB, unsigned Reg) {
+    MIB.addUse(Reg);
+  }
 
-  unsigned getRegFromArg(const MachineInstrBuilder &MIB) {
-    return MIB->getOperand(0).getReg();
+  void addUseFromArg(MachineInstrBuilder &MIB, const MachineInstrBuilder &UseMIB) {
+    MIB.addUse(UseMIB->getOperand(0).getReg());
   }
 
+  void addUsesFromArgs(MachineInstrBuilder &MIB) { }
+  template<typename UseArgTy, typename ... UseArgsTy>
+  void addUsesFromArgs(MachineInstrBuilder &MIB, UseArgTy &&Arg1, UseArgsTy &&... Args) {
+    addUseFromArg(MIB, Arg1);
+    addUsesFromArgs(std::forward<UseArgsTy>(Args)...);
+  }
 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:
   /// \Ty Either LLT/TargetRegisterClass/unsigned types for Dst
   /// \Args Variadic list of uses of types(unsigned/MachineInstrBuilder)
   /// Uses of type MachineInstrBuilder will perform
   /// getOperand(0).getReg() to convert to register.
   template <typename DstTy, typename... UseArgsTy>
   MachineInstrBuilder buildInstr(unsigned Opc, DstTy &&Ty,
                                  UseArgsTy &&... Args) {
     auto MIB = buildInstr(Opc).addDef(getDestFromArg(Ty));
-    unsigned It[] = {(getRegFromArg(Args))...};
-    for (const auto &i : It)
-      MIB.addUse(i);
+    addUsesFromArgs(MIB, std::forward<UseArgsTy>(Args)...);
     return MIB;
   }
 
   /// Build but don't insert <empty> = \p Opcode <empty>.
   ///
   /// \pre setMF, setBasicBlock or setMI  must have been called.
   ///
   /// \return a MachineInstrBuilder for the newly created instruction.