[PATCH] D156346: CodeGen: Disable isCopyInstrImpl if there are implicit operands

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1044
     if (MI.isCopy()) {
+      // TODO: Should validate implicit operands here?
       return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
----------------
arsenm wrote:
> qcolombet wrote:
> > Maybe that should be part of the verifier?
> Hypothetically you can still end up with the same implicit-def case as the mov, if you were to coalesce a copy into SUBREG_TO_REG. I believe all the instances which use SUBREG_TO_REG are fed by non-copy sources, so not sure if this happens in practice. The verifier probably should verify there are no implicit-defs at least
> 
> 
> Hypothetically you can still end up with the same implicit-def case as the mov

True.

The scary thing to me is if regular copies have implicit defs, it would be good to distinguish if it is to make the liveness accurate or if it is something that went south (e.g., a target specific operation that got merged with a regular generic copy, in that case, codegen may be broken.)


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1053
+    // instruction with an undef subregister def, and a full register
+    // implicit-def appended to the operand list.
+
----------------
arsenm wrote:
> qcolombet wrote:
> > Like I said in https://reviews.llvm.org/D156345, there was already a distinction between pure copies (`isCopy`) and copies with additional stuff that may require careful handling (`isCopyLike`).
> > 
> > Following this distinction, I feel that either:
> > - this is the right fix, or
> > - we need to make all the places that deals with isCopyInstr robust w.r.t. `isCopyLike` semantic, which may not be worth it to pull
> > worth it to pull
> 
> pull what?
> pull what?

Pull off :P.

I mean it may not be worth spending a lot of effort to support non pure copy everywhere.
I.e., Heroically try to implement the full support everywhere for a negligible gain.


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

https://reviews.llvm.org/D156346