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

[Diana Picus via Phabricator via llvm-commits]

rovka created this revision.
rovka added reviewers: AMDGPU, paquette, aemerson.
Herald added a subscriber: hiraditya.
Herald added a project: All.
rovka requested review of this revision.
Herald added a project: LLVM.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140964

Files:
  llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h
  llvm/lib/CodeGen/GlobalISel/MachineIRBuilder.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D140964.486212.patch
Type: text/x-patch
Size: 5043 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230104/cd09ce96/attachment.bin>