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

[rovka]

Summary

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 to 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).

[foad]

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.

I like this!

Does this patch actually fix any missed CSE opportunities?

I wonder if we could get rid of buildMerge altogether, in favour of separate buildMergeValues / buildConcatVectors / buildBuildVector functions.

[rovka]

In D140964#4025539, @foad wrote:

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.

I like this!

Does this patch actually fix any missed CSE opportunities?

No, we also need to add G_BUILD_VECTOR to CSE.

I wonder if we could get rid of buildMerge altogether, in favour of separate buildMergeValues / buildConcatVectors / buildBuildVector functions.

We already have buildBuildVector and buildConcatVectors and they seem to have a decent number of uses. I haven't really audited all the uses of buildMerge to see if it would be onerous to replicate the checks for vectors everywhere :) I could have a look and send a follow-up patch if the existing uses aren't too sloppy.

[arsenm]

We should have a separate buildMergeValues and buildMerge could use a less confusing name

[rovka]

In D140964#4026229, @arsenm wrote:

We should have a separate buildMergeValues and buildMerge could use a less confusing name

Ah, naming... I'll see what I can do :)

Thanks for the reviews!

llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h

 /// Helper class to build MachineInstr.
 /// It keeps internally the insertion point and debug location for all
 /// the new instructions we want to create.
 /// This information can be modify via the related setters.
 class MachineIRBuilder {
 
   MachineIRBuilderState State;
 
+  unsigned getOpcodeForMerge(const DstOp &DstOp, ArrayRef<SrcOp> SrcOps) const;
+
 protected:
   void validateTruncExt(const LLT Dst, const LLT Src, bool IsExtend);
 
   void validateUnaryOp(const LLT Res, const LLT Op0);
   void validateBinaryOp(const LLT Res, const LLT Op0, const LLT Op1);
   void validateShiftOp(const LLT Res, const LLT Op0, const LLT Op1);
 
   void validateSelectOp(const LLT ResTy, const LLT TstTy, const LLT Op0Ty,
public:
   ///
   /// \return a MachineInstrBuilder for the newly created instruction.
   MachineInstrBuilder buildExtract(const DstOp &Res, const SrcOp &Src, uint64_t Index);
 
   /// Build and insert \p Res = IMPLICIT_DEF.
   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);
 
   /// Build and insert \p Res0, ... = G_UNMERGE_VALUES \p Op
   ///
   /// G_UNMERGE_VALUES splits contiguous bits of the input into multiple
   ///

llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

 
 MachineInstrBuilder MachineIRBuilder::buildMerge(const DstOp &Res,
                                                  ArrayRef<Register> Ops) {
   // Unfortunately to convert from ArrayRef<LLT> to ArrayRef<SrcOp>,
   // 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());
   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,
                                                    const SrcOp &Op) {
   // Unfortunately to convert from ArrayRef<LLT> to ArrayRef<DstOp>,
   // we need some temporary storage for the DstOp objects. Here we use a
   // sufficiently large SmallVector to not go through the heap.
   SmallVector<DstOp, 8> TmpVec(Res.begin(), Res.end());

 
 MachineInstrBuilder
 MachineIRBuilder::buildBuildVectorTrunc(const DstOp &Res,
                                         ArrayRef<Register> Ops) {
   // Unfortunately to convert from ArrayRef<Register> to ArrayRef<SrcOp>,
   // 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);
 }
 
 MachineInstrBuilder MachineIRBuilder::buildShuffleSplat(const DstOp &Res,
                                                         const SrcOp &Src) {
   LLT DstTy = Res.getLLTTy(*getMRI());
   assert(Src.getLLTTy(*getMRI()) == DstTy.getElementType() &&
          "Expected Src to match Dst elt ty");
assert(llvm::all_of(DstOps,
            "type mismatch in output list");
     assert((TypeSize::ScalarTy)DstOps.size() *
                    DstOps[0].getLLTTy(*getMRI()).getSizeInBits() ==
                SrcOps[0].getLLTTy(*getMRI()).getSizeInBits() &&
            "input operands do not cover output register");
     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) {
                           return Op.getLLTTy(*getMRI()) ==
                                  SrcOps[0].getLLTTy(*getMRI());
                         }) &&
            "type mismatch in input list");
     assert((TypeSize::ScalarTy)SrcOps.size() *
                    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: {
     assert(DstOps.size() == 1 && "Invalid Dst size");
     assert(SrcOps.size() == 2 && "Invalid Src size");
     assert(SrcOps[0].getLLTTy(*getMRI()).isVector() && "Invalid operand type");
     assert((DstOps[0].getLLTTy(*getMRI()).isScalar() ||
             DstOps[0].getLLTTy(*getMRI()).isPointer()) &&
case TargetOpcode::G_BUILD_VECTOR_TRUNC: {
     assert(DstOps[0].getLLTTy(*getMRI()).isVector() &&
            "Res type must be a vector");
     assert(llvm::all_of(SrcOps,
                         [&, this](const SrcOp &Op) {
                           return Op.getLLTTy(*getMRI()) ==
                                  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: {
     assert(DstOps.size() == 1 && "Invalid DstOps");
     assert((!SrcOps.empty() || SrcOps.size() < 2) &&
            "Must have at least 2 operands");
     assert(llvm::all_of(SrcOps,
                         [&, this](const SrcOp &Op) {