[GlobalISel] Don't switch opcodes in MIRBuilder::buildInstr

At the moment, `MachineIRBuilder::buildInstr` may build an instruction
with a different opcode than the one passed in as parameter. This may
cause confusion for its consumers, such as `CSEMIRBuilder`, which will
memoize the instruction based on the new opcode, but will search
through the memoized instructions based on the original one (resulting
in missed CSE opportunities). This is all the more unpleasant since
buildInstr is virtual and may call itself recursively both directly
and via buildCast, so it's not always easy to follow what's going on.

This patch simplifies the API of `MachineIRBuilder` so that the `buildInstr`
method does the least surprising thing (i.e. builds an instruction with
the specified opcode) and only the convenience `buildX` methods
(`buildMerge` etc) are allowed freedom over which opcode to use. This can
still be confusing (e.g. one might write a unit test using
`buildBuildVectorTrunc` but instead get a plain `G_BUILD_VECTOR`), but at
least it's explained in the comments.

In practice, this boils down to 3 changes:
* `buildInstr(G_MERGE_VALUES)` will no longer call itself with
`G_BUILD_VECTOR` or `G_CONCAT_VECTORS`; this functionality is moved to
`buildMerge` and replaced with an assert;
* `buildInstr(G_BUILD_VECTOR_TRUNC)` will no longer call itself with
`G_BUILD_VECTOR`; this functionality is moved to `buildBuildVectorTrunc`
and replaced with an assert;
* `buildInstr(G_MERGE_VALUES)` will no longer call `buildCast` and will
instead assert if we're trying to merge a single value; no change is
needed in `buildMerge` since it was already asserting more than one
source operand.

This change is NFC for users of the `buildX` methods, but users that
call `buildInstr` with relaxed parameters will have to update their code
(such instances will hopefully be easy to find thanks to the asserts).

Differential Revision: https://reviews.llvm.org/D140964

2 files changed, 29 insertions, 15 deletions

diff --git a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
index f05d491..b11ffcf 100644
--- a/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
+++ b/llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
@@ -222,6 +222,8 @@ class MachineIRBuilder {
 
   MachineIRBuilderState State;
 
+  unsigned getOpcodeForMerge(const DstOp &DstOp, ArrayRef<SrcOp> SrcOps) const;
+
 protected:
   void validateTruncExt(const LLT Dst, const LLT Src, bool IsExtend);
 
@@ -966,16 +968,22 @@ public:
   MachineInstrBuilder buildUndef(const DstOp &Res);
 
   /// Build and insert \p Res = G_MERGE_VALUES \p Op0, ...
+  ///               or \p Res = G_BUILD_VECTOR \p Op0, ...
+  ///               or \p Res = G_CONCAT_VECTORS \p Op0, ...
   ///
   /// G_MERGE_VALUES combines the input elements contiguously into a larger
-  /// register.
+  /// register. It is used when the destination register is not a vector.
+  /// G_BUILD_VECTOR combines scalar inputs into a vector register.
+  /// G_CONCAT_VECTORS combines vector inputs into a vector register.
   ///
   /// \pre setBasicBlock or setMI must have been called.
   /// \pre The entire register \p Res (and no more) must be covered by the input
   ///      registers.
   /// \pre The type of all \p Ops registers must be identical.
   ///
-  /// \return a MachineInstrBuilder for the newly created instruction.
+  /// \return a MachineInstrBuilder for the newly created instruction. The
+  ///         opcode of the new instruction will depend on the types of both
+  ///         the destination and the sources.
   MachineInstrBuilder buildMerge(const DstOp &Res, ArrayRef<Register> Ops);
   MachineInstrBuilder buildMerge(const DstOp &Res,
                                  std::initializer_list<SrcOp> Ops);
diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
index 44fb5db..726f9b4 100644
--- a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp
@@ -604,14 +604,25 @@ MachineInstrBuilder MachineIRBuilder::buildMerge(const DstOp &Res,
   // sufficiently large SmallVector to not go through the heap.
   SmallVector<SrcOp, 8> TmpVec(Ops.begin(), Ops.end());
   assert(TmpVec.size() > 1);
-  return buildInstr(TargetOpcode::G_MERGE_VALUES, Res, TmpVec);
+  return buildInstr(getOpcodeForMerge(Res, TmpVec), Res, TmpVec);
 }
 
 MachineInstrBuilder
 MachineIRBuilder::buildMerge(const DstOp &Res,
                              std::initializer_list<SrcOp> Ops) {
   assert(Ops.size() > 1);
-  return buildInstr(TargetOpcode::G_MERGE_VALUES, Res, Ops);
+  return buildInstr(getOpcodeForMerge(Res, Ops), Res, Ops);
+}
+
+unsigned MachineIRBuilder::getOpcodeForMerge(const DstOp &DstOp,
+                                             ArrayRef<SrcOp> SrcOps) const {
+  if (DstOp.getLLTTy(*getMRI()).isVector()) {
+    if (SrcOps[0].getLLTTy(*getMRI()).isVector())
+      return TargetOpcode::G_CONCAT_VECTORS;
+    return TargetOpcode::G_BUILD_VECTOR;
+  }
+
+  return TargetOpcode::G_MERGE_VALUES;
 }
 
 MachineInstrBuilder MachineIRBuilder::buildUnmerge(ArrayRef<LLT> Res,
@@ -674,6 +685,9 @@ MachineIRBuilder::buildBuildVectorTrunc(const DstOp &Res,
   // we need some temporary storage for the DstOp objects. Here we use a
   // sufficiently large SmallVector to not go through the heap.
   SmallVector<SrcOp, 8> TmpVec(Ops.begin(), Ops.end());
+  if (TmpVec[0].getLLTTy(*getMRI()).getSizeInBits() ==
+      Res.getLLTTy(*getMRI()).getElementType().getSizeInBits())
+    return buildInstr(TargetOpcode::G_BUILD_VECTOR, Res, TmpVec);
   return buildInstr(TargetOpcode::G_BUILD_VECTOR_TRUNC, Res, TmpVec);
 }
 
@@ -1159,7 +1173,7 @@ MachineIRBuilder::buildInstr(unsigned Opc, ArrayRef<DstOp> DstOps,
     break;
   }
   case TargetOpcode::G_MERGE_VALUES: {
-    assert(!SrcOps.empty() && "invalid trivial sequence");
+    assert(SrcOps.size() >= 2 && "invalid trivial sequence");
     assert(DstOps.size() == 1 && "Invalid Dst");
     assert(llvm::all_of(SrcOps,
                         [&, this](const SrcOp &Op) {
@@ -1171,13 +1185,8 @@ MachineIRBuilder::buildInstr(unsigned Opc, ArrayRef<DstOp> DstOps,
                    SrcOps[0].getLLTTy(*getMRI()).getSizeInBits() ==
                DstOps[0].getLLTTy(*getMRI()).getSizeInBits() &&
            "input operands do not cover output register");
-    if (SrcOps.size() == 1)
-      return buildCast(DstOps[0], SrcOps[0]);
-    if (DstOps[0].getLLTTy(*getMRI()).isVector()) {
-      if (SrcOps[0].getLLTTy(*getMRI()).isVector())
-        return buildInstr(TargetOpcode::G_CONCAT_VECTORS, DstOps, SrcOps);
-      return buildInstr(TargetOpcode::G_BUILD_VECTOR, DstOps, SrcOps);
-    }
+    assert(!DstOps[0].getLLTTy(*getMRI()).isVector() &&
+           "vectors should be built with G_CONCAT_VECTOR or G_BUILD_VECTOR");
     break;
   }
   case TargetOpcode::G_EXTRACT_VECTOR_ELT: {
@@ -1237,9 +1246,6 @@ MachineIRBuilder::buildInstr(unsigned Opc, ArrayRef<DstOp> DstOps,
                                  SrcOps[0].getLLTTy(*getMRI());
                         }) &&
            "type mismatch in input list");
-    if (SrcOps[0].getLLTTy(*getMRI()).getSizeInBits() ==
-        DstOps[0].getLLTTy(*getMRI()).getElementType().getSizeInBits())
-      return buildInstr(TargetOpcode::G_BUILD_VECTOR, DstOps, SrcOps);
     break;
   }
   case TargetOpcode::G_CONCAT_VECTORS: {