[PATCH] D55294: [GISel]: Refactor MachineIRBuilder to allow passing additional parameters to build Instrs

[Justin Bogner via Phabricator via llvm-commits]

bogner added a comment.

This looks like a big improvement to the API overall, thanks for working on it.

You're a bit inconsistent throughout the implementation of MachineIRBuilder on whether you replace the implementations of buildFoo() methods with calls to buildInstr or not - is there a reason not to make them all forward to the generic implementation?



================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:65
+public:
+  enum DstType { Ty_LLT, Ty_Reg, Ty_RC };
+  DstOp(unsigned R) : Reg(R), Ty(DstType::Ty_Reg) {}
----------------
Does this need to be public? I think it's only used internally to DstOp.

Also, this can probably be an enum class easily enough, since you don't convert it to or from an integer type ever.


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:88-89
+    switch (Ty) {
+    default:
+      return LLT{};
+    case Ty_LLT:
----------------
It would be clearer to do "case DstType::TyRC" here explicitly, since that's the only case the default catches. OTOH, should it even be legal to call this if you have a register class? Maybe it should assert.


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:110
+public:
+  enum SrcType { Ty_Reg, Ty_MIB };
+  SrcOp(unsigned R) : Reg(R), Ty(SrcType::Ty_Reg) {}
----------------
Same comments as on DstType


================
Comment at: include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:115
+  void addSrcToMIB(MachineInstrBuilder &MIB) const {
+    if (Ty == SrcType::Ty_Reg)
+      MIB.addUse(Reg);
----------------
I'd probably do a switch statement here, even though it's more verbose. That way if we ever add another SrcType it will warn if we forget to update it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55294