From 22924bd48dcea4ba23caccedbb366fd2b4e66e30 Mon Sep 17 00:00:00 2001 From: Diana Picus Date: Wed, 4 Jan 2023 11:59:49 +0100 Subject: [PATCH] [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 --- .../CodeGen/GlobalISel/MachineIRBuilder.h | 12 +++++-- .../CodeGen/GlobalISel/MachineIRBuilder.cpp | 32 +++++++++++-------- 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 f05d49194c31..b11ffcfd70e2 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 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 Ops); MachineInstrBuilder buildMerge(const DstOp &Res, std::initializer_list Ops); diff --git a/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp index 44fb5db43663..726f9b47de78 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 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 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 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 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 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 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 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 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: {