CodeGen: Disable isCopyInstrImpl if there are implicit operands

This is a conservative workaround for broken liveness tracking of
SUBREG_TO_REG to speculatively fix all targets. The current reported
failures are on X86 only, but this issue should appear for all targets
that use SUBREG_TO_REG. The next minimally correct refinement would be
to disallow only implicit defs.

The coalescer now introduces implicit-defs of the super register to
track the dependency on other subregisters. If we see such an implicit
operand, we cannot simply treat the subregister def as the result
operand in case downstream users depend on the implicitly defined
parts. Really target implementations should be considering the
implicit defs and trying to interpret them appropriately (maybe with
some generic helpers). The full implicit def could possibly be
reported as the move result, rather than the subregister def but that
requires additional work.

Hopefully fixes #64060 as well.

This needs to be applied to the release branch.

https://reviews.llvm.org/D156346

Author: arsenm

llvm/include/llvm/CodeGen/TargetInstrInfo.h

@@ -1036,10 +1036,29 @@ class TargetInstrInfo : public MCInstrInfo {
   /// For COPY-instruction the method naturally returns destination and source
   /// registers as machine operands, for all other instructions the method calls
   /// target-dependent implementation.
-  std::optional<DestSourcePair> isCopyInstr(const MachineInstr &MI) const {
+  std::optional<DestSourcePair>
+  isCopyInstr(const MachineInstr &MI,
+              bool ForbidImplicitOperands = true) const {
     if (MI.isCopy()) {
+      // TODO: Should validate implicit operands here?
       return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
     }
+
+    // TODO: This is a conservative hack to ensure correctness when extra
+    // operands are added for special liveness tracking, while also not changing
+    // debug info. In particular SUBREG_TO_REG may introduce an implicit-def of
+    // a super register after coalescing. This may manifest as a copy-like
+    // instruction with an undef subregister def, and a full register
+    // implicit-def appended to the operand list.
+
+    // Really, implementations of this should be considering extra implicit
+    // operands. A more sophisticated implementation would recognize an
+    // implicit-def of the full register, and report that as the
+    // destination. This should be removed when all targets are validated for
+    // correct SUBREG_TO_REG liveness handling.
+    if (ForbidImplicitOperands && MI.getNumImplicitOperands() != 0)
+      return std::nullopt;
+
     return isCopyInstrImpl(MI);
   }
 

llvm/lib/CodeGen/LiveDebugValues/InstrRefBasedImpl.cpp

@@ -2116,7 +2116,7 @@ bool InstrRefBasedLDV::transferSpillOrRestoreInst(MachineInstr &MI) {
 }
 
 bool InstrRefBasedLDV::transferRegisterCopy(MachineInstr &MI) {
-  auto DestSrc = TII->isCopyInstr(MI);
+  auto DestSrc = TII->isCopyInstr(MI, false);
   if (!DestSrc)
     return false;
 

llvm/lib/CodeGen/LiveDebugValues/VarLocBasedImpl.cpp

@@ -1364,7 +1364,7 @@ void VarLocBasedLDV::removeEntryValue(const MachineInstr &MI,
     // TODO: Try to keep tracking of an entry value if we encounter a propagated
     // DBG_VALUE describing the copy of the entry value. (Propagated entry value
     // does not indicate the parameter modification.)
-    auto DestSrc = TII->isCopyInstr(*TransferInst);
+    auto DestSrc = TII->isCopyInstr(*TransferInst, false);
     if (DestSrc) {
       const MachineOperand *SrcRegOp, *DestRegOp;
       SrcRegOp = DestSrc->Source;
@@ -1840,7 +1840,7 @@ void VarLocBasedLDV::transferRegisterCopy(MachineInstr &MI,
                                            OpenRangesSet &OpenRanges,
                                            VarLocMap &VarLocIDs,
                                            TransferMap &Transfers) {
-  auto DestSrc = TII->isCopyInstr(MI);
+  auto DestSrc = TII->isCopyInstr(MI, false);
   if (!DestSrc)
     return;
 

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

@@ -9176,7 +9176,7 @@ static std::optional<ParamLoadedValue>
 describeORRLoadedValue(const MachineInstr &MI, Register DescribedReg,
                        const TargetInstrInfo *TII,
                        const TargetRegisterInfo *TRI) {
-  auto DestSrc = TII->isCopyInstr(MI);
+  auto DestSrc = TII->isCopyInstr(MI, false);
   if (!DestSrc)
     return std::nullopt;