[PATCH] D59444: [GlobalISel] Change MachineIRBuilder's SrcOp to contain subregister info

[Amara Emerson via Phabricator via llvm-commits]

aemerson abandoned this revision.
aemerson added a comment.

In D59444#1433415 <https://reviews.llvm.org/D59444#1433415>, @aditya_nandakumar wrote:

> In general I'm not too fond of building very target specific instructions involving subregs with the MachineIRBuilder (As these are only likely to be used in a few places in the selector). Specifically for this case of building a subreg, you can say
>
>   Builder.buildInstr(COPY, {Dst},{})
>     .addReg(Src, 0, SubReg);
>
>
> which is not too much larger than (and doesn't require making every SrcOp bigger).
>
>   Builder.buildInstr(COPY, {Dst}, {{Src, SubReg}});
>
>
> While adding CSE support for COPYs with Subregs is great, I'm not sure how often it'll trigger and be useful (If it does occur then great). Also should we start adding support for building other kinds of target instructions - Adding immediate operands/target index nodes etc?
>  My approach for building non generic instructions have been with using the .addImm(..).addReg(...)... pattern - but if others strongly feel the need to add the ability to pass in all of the operands at once to the builder interfaces, and enabling CSE for them, then we can go ahead with this change.


In that case I'll revert r356304 keeping in mind that we should take care not to enable COPY CSE in the selector in future, as it won't be subreg safe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59444/new/

https://reviews.llvm.org/D59444