[all-commits] [llvm/llvm-project] 22924b: [GlobalISel] Don't switch opcodes in MIRBuilder::b...

[Diana via All-commits]

  Branch: refs/heads/main
  Home:   https://github.com/llvm/llvm-project
  Commit: 22924bd48dcea4ba23caccedbb366fd2b4e66e30
      https://github.com/llvm/llvm-project/commit/22924bd48dcea4ba23caccedbb366fd2b4e66e30
  Author: Diana Picus <Diana-Magda.Picus at amd.com>
  Date:   2023-01-05 (Thu, 05 Jan 2023)

  Changed paths:
    M llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
    M llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

  Log Message:
  -----------
  [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