[PATCH] D143753: [MachineInstr] Introduce TII buildCopy helper functions (NFC).

Thu Feb 16 08:32:45 PST 2023

cdevadas added a comment.

In D143753#4131294 <https://reviews.llvm.org/D143753#4131294>, @qcolombet wrote:

> Hey,
>
> What's the long term plan for this helper?
>
> I am not mad at the refactoring itself, but I'm wondering what kind of criteria we should use to decide when we add such helper.

The real motivation for these buildCopy helper functions is to minimize the additional code changes required while introducing getCopyOpcode() as part of D143754 <https://reviews.llvm.org/D143754>.
One could probably say that "still you could have avoided the buildCopy functions, and instead replace TargetOpcode::COPY with getCopyOpcode()". 
I think it would be ok to refactor the code if found an opportunity while working around that code.

> For instance, should we do the same thing for `PHI`, `INSERT_SUBREG`, etc. and why or why not.
> Also, should these helper be virtual or not?

As I said earlier, COPY wasn't a random pick, but rather a code refactor for another patch.

> I guess where I am going is what do you intend to simplify/achieve with them?

I felt it was better with `buildCopy` rather than the direct replacement of `TargetOpcode::COPY` with getCopyOpcode() in their original `BuildMI` instances. You tend to agree with that towards the end - "(I actually like the patch)".

> The bottom line is without a proper rationale on why we need this helper, I feel that it gets difficult to know which method should be used to construct a copy.
> Right now we have `BuildMI` (and the underlying `MachineInstrBuilder` object that can be called directly), `MachineFunction::CreateMachineInstr`, `MachineIRBuilder::buildInstr` (BTW `MachineIRBuilder` has its own `MachineIRBuilder::buildCopy`) and we add a new method here.

I agree. All my buildCopy instances currently return MachineInstr*. I wasn't sure they should return MachineInstrBuilder or any other form as you mentioned. It can be worked out if anyone has a better suggestion.
And about MachineIRBuilder::buildCopy, I feel it is better to retain this very specific instance considering the actual function being called ( the generic MachineIRBuilder::buildInstr) and the operands used which are different from the instances I added.

> I believe we need to disambiguate when this one should be used and why.
>
> Admittedly I'm playing the devil's advocate here (I actually like the patch) but I think it is important to flush out the goals and intended usages to be able to better guide future users of this (and the other) API(s).
>
> Cheers,
> -Quentin

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143753