[mcp-frameinst: 1/3]: [MCP][NFC] Cleanup and prepare to preserve frame-setup/destroy

[slinder1]

This mixes renames, removing redundant code, avoiding
else-after-return, etc. with factoring out the isNeverRedundant
concept.

Change-Id: I43a62a9415019cdd63c68fd3b915ebb7505d317a

---

Stack:

• [mcp-frameinst: 3/3]: [MCP] Never eliminate frame-setup/destroy instructions #186237
• [mcp-frameinst: 2/3]: [MCP][NFC] Opinionated refactoring #186239
• [mcp-frameinst: 1/3]: [MCP][NFC] Cleanup and prepare to preserve frame-setup/destroy #186240⬅
• main

_{(Note: Closed and merged PRs may not be reflected here and PR numbering is not stable.)}

[llvmbot]

@llvm/pr-subscribers-llvm-regalloc

Author: Scott Linder (slinder1)

Changes

This mixes renames, removing redundant code, avoiding
else-after-return, etc. with factoring out the isNeverRedundant
concept. I can drop any bits people would rather I not touch.

Change-Id: I43a62a9415019cdd63c68fd3b915ebb7505d317a

---

Stack:

• #186237
• #186239
• #186240⬅
• #186238
• main

_{(Note: Closed and merged PRs may not be reflected here and PR numbering is not stable.)}

---

Full diff: https://github.com/llvm/llvm-project/pull/186240.diff

1 Files Affected:

• (modified) llvm/lib/CodeGen/MachineCopyPropagation.cpp (+73-61)

diff --git a/llvm/lib/CodeGen/MachineCopyPropagation.cpp b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
index 58ff6f77830b3..258fa607477df 100644
--- a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
+++ b/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -101,8 +101,7 @@ static std::optional<DestSourcePair> isCopyInstr(const MachineInstr &MI,
     return TII.isCopyInstr(MI);
 
   if (MI.isCopy())
-    return std::optional<DestSourcePair>(
-        DestSourcePair{MI.getOperand(0), MI.getOperand(1)});
+    return DestSourcePair{MI.getOperand(0), MI.getOperand(1)};
 
   return std::nullopt;
 }
@@ -129,19 +128,17 @@ class CopyTracker {
                                   const TargetRegisterInfo &TRI) {
     const uint32_t *RegMask = RegMaskOp.getRegMask();
     auto [It, Inserted] = RegMaskToPreservedRegUnits.try_emplace(RegMask);
-    if (!Inserted) {
+    if (!Inserted)
       return It->second;
-    } else {
-      BitVector &PreservedRegUnits = It->second;
+    BitVector &PreservedRegUnits = It->second;
 
-      PreservedRegUnits.resize(TRI.getNumRegUnits());
-      for (unsigned SafeReg = 0, E = TRI.getNumRegs(); SafeReg < E; ++SafeReg)
-        if (!RegMaskOp.clobbersPhysReg(SafeReg))
-          for (MCRegUnit SafeUnit : TRI.regunits(SafeReg))
-            PreservedRegUnits.set(static_cast<unsigned>(SafeUnit));
+    PreservedRegUnits.resize(TRI.getNumRegUnits());
+    for (unsigned SafeReg = 0, E = TRI.getNumRegs(); SafeReg < E; ++SafeReg)
+      if (!RegMaskOp.clobbersPhysReg(SafeReg))
+        for (MCRegUnit SafeUnit : TRI.regunits(SafeReg))
+          PreservedRegUnits.set(static_cast<unsigned>(SafeUnit));
 
-      return PreservedRegUnits;
-    }
+    return PreservedRegUnits;
   }
 
   /// Mark all of the given registers and their subregisters as unavailable for
@@ -226,10 +223,10 @@ class CopyTracker {
           if (SrcCopy != Copies.end() && SrcCopy->second.LastSeenUseInCopy) {
             // If SrcCopy defines multiple values, we only need
             // to erase the record for Def in DefRegs.
-            for (auto itr = SrcCopy->second.DefRegs.begin();
-                 itr != SrcCopy->second.DefRegs.end(); itr++) {
-              if (*itr == Def) {
-                SrcCopy->second.DefRegs.erase(itr);
+            for (auto *Itr = SrcCopy->second.DefRegs.begin();
+                 Itr != SrcCopy->second.DefRegs.end(); Itr++) {
+              if (*Itr == Def) {
+                SrcCopy->second.DefRegs.erase(Itr);
                 // If DefReg becomes empty after removal, we can remove the
                 // SrcCopy from the tracker's copy maps. We only remove those
                 // entries solely record the Def is defined by Src. If an
@@ -467,11 +464,11 @@ class MachineCopyPropagation {
 private:
   typedef enum { DebugUse = false, RegularUse = true } DebugType;
 
-  void ReadRegister(MCRegister Reg, MachineInstr &Reader, DebugType DT);
+  void readRegister(MCRegister Reg, MachineInstr &Reader, DebugType DT);
   void readSuccessorLiveIns(const MachineBasicBlock &MBB);
-  void ForwardCopyPropagateBlock(MachineBasicBlock &MBB);
-  void BackwardCopyPropagateBlock(MachineBasicBlock &MBB);
-  void EliminateSpillageCopies(MachineBasicBlock &MBB);
+  void forwardCopyPropagateBlock(MachineBasicBlock &MBB);
+  void backwardCopyPropagateBlock(MachineBasicBlock &MBB);
+  void eliminateSpillageCopies(MachineBasicBlock &MBB);
   bool eraseIfRedundant(MachineInstr &Copy, MCRegister Src, MCRegister Def);
   void forwardUses(MachineInstr &MI);
   void propagateDefs(MachineInstr &MI);
@@ -480,9 +477,24 @@ class MachineCopyPropagation {
   bool isBackwardPropagatableRegClassCopy(const MachineInstr &Copy,
                                           const MachineInstr &UseI,
                                           unsigned UseIdx);
+  bool isBackwardPropagatableCopy(const MachineInstr &Copy,
+                                  const DestSourcePair &CopyOperands);
+  bool isNeverRedundant(MCRegister CopyOperand) {
+    // Avoid eliminating a copy from/to a reserved registers as we cannot
+    // predict the value (Example: The sparc zero register is writable but stays
+    // zero).
+    return MRI->isReserved(CopyOperand);
+  }
+  bool isNeverRedundant(const MachineInstr &Copy) { return false; }
+  bool isNeverRedundant(const MachineInstr &Copy,
+                        const DestSourcePair &CopyOperands) {
+    return isNeverRedundant(Copy) ||
+           isNeverRedundant(CopyOperands.Destination->getReg().asMCReg()) ||
+           isNeverRedundant(CopyOperands.Source->getReg().asMCReg());
+  }
   bool hasImplicitOverlap(const MachineInstr &MI, const MachineOperand &Use);
   bool hasOverlappingMultipleDef(const MachineInstr &MI,
-                                 const MachineOperand &MODef, Register Def);
+                                 const MachineOperand &MODef, MCRegister Def);
   bool canUpdateSrcUsers(const MachineInstr &Copy,
                          const MachineOperand &CopySrc);
 
@@ -527,7 +539,7 @@ char &llvm::MachineCopyPropagationID = MachineCopyPropagationLegacy::ID;
 INITIALIZE_PASS(MachineCopyPropagationLegacy, DEBUG_TYPE,
                 "Machine Copy Propagation Pass", false, false)
 
-void MachineCopyPropagation::ReadRegister(MCRegister Reg, MachineInstr &Reader,
+void MachineCopyPropagation::readRegister(MCRegister Reg, MachineInstr &Reader,
                                           DebugType DT) {
   // If 'Reg' is defined by a copy, the copy is no longer a candidate
   // for elimination. If a copy is "read" by a debug user, record the user
@@ -590,9 +602,7 @@ static bool isNopCopy(const MachineInstr &PreviousCopy, MCRegister Src,
 /// copying the super registers.
 bool MachineCopyPropagation::eraseIfRedundant(MachineInstr &Copy,
                                               MCRegister Src, MCRegister Def) {
-  // Avoid eliminating a copy from/to a reserved registers as we cannot predict
-  // the value (Example: The sparc zero register is writable but stays zero).
-  if (MRI->isReserved(Src) || MRI->isReserved(Def))
+  if (isNeverRedundant(Copy) || isNeverRedundant(Src) || isNeverRedundant(Def))
     return false;
 
   // Search for an existing copy.
@@ -616,7 +626,7 @@ bool MachineCopyPropagation::eraseIfRedundant(MachineInstr &Copy,
       isCopyInstr(Copy, *TII, UseCopyInstr);
   assert(CopyOperands);
 
-  Register CopyDef = CopyOperands->Destination->getReg();
+  MCRegister CopyDef = CopyOperands->Destination->getReg().asMCReg();
   assert(CopyDef == Src || CopyDef == Def);
   for (MachineInstr &MI :
        make_range(PrevCopy->getIterator(), Copy.getIterator()))
@@ -649,6 +659,20 @@ bool MachineCopyPropagation::isBackwardPropagatableRegClassCopy(
   return false;
 }
 
+bool MachineCopyPropagation::isBackwardPropagatableCopy(
+    const MachineInstr &Copy, const DestSourcePair &CopyOperands) {
+  MCRegister Def = CopyOperands.Destination->getReg().asMCReg();
+  MCRegister Src = CopyOperands.Source->getReg().asMCReg();
+
+  if (!Def || !Src)
+    return false;
+
+  if (isNeverRedundant(Copy, CopyOperands))
+    return false;
+
+  return CopyOperands.Source->isRenamable() && CopyOperands.Source->isKill();
+}
+
 /// Decide whether we should forward the source of \param Copy to its use in
 /// \param UseI based on the physical register class constraints of the opcode
 /// and avoiding introducing more cross-class COPYs.
@@ -739,7 +763,7 @@ bool MachineCopyPropagation::hasImplicitOverlap(const MachineInstr &MI,
 /// For example, on ARM: umull   r9, r9, lr, r0
 /// The umull instruction is unpredictable unless RdHi and RdLo are different.
 bool MachineCopyPropagation::hasOverlappingMultipleDef(
-    const MachineInstr &MI, const MachineOperand &MODef, Register Def) {
+    const MachineInstr &MI, const MachineOperand &MODef, MCRegister Def) {
   for (const MachineOperand &MIDef : MI.all_defs()) {
     if ((&MIDef != &MODef) && MIDef.isReg() &&
         TRI->regsOverlap(Def, MIDef.getReg()))
@@ -807,11 +831,11 @@ void MachineCopyPropagation::forwardUses(MachineInstr &MI) {
 
     std::optional<DestSourcePair> CopyOperands =
         isCopyInstr(*Copy, *TII, UseCopyInstr);
-    Register CopyDstReg = CopyOperands->Destination->getReg();
+    MCRegister CopyDstReg = CopyOperands->Destination->getReg().asMCReg();
     const MachineOperand &CopySrc = *CopyOperands->Source;
-    Register CopySrcReg = CopySrc.getReg();
+    MCRegister CopySrcReg = CopySrc.getReg().asMCReg();
 
-    Register ForwardedReg = CopySrcReg;
+    MCRegister ForwardedReg = CopySrcReg;
     // MI might use a sub-register of the Copy destination, in which case the
     // forwarded register is the matching sub-register of the Copy source.
     if (MOUse.getReg() != CopyDstReg) {
@@ -874,7 +898,7 @@ void MachineCopyPropagation::forwardUses(MachineInstr &MI) {
   }
 }
 
-void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
+void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
   LLVM_DEBUG(dbgs() << "MCP: ForwardCopyPropagateBlock " << MBB.getName()
                     << "\n");
 
@@ -920,7 +944,7 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
         // instruction, so we need to make sure we don't remove it as dead
         // later.
         if (MO.isTied())
-          ReadRegister(Reg, MI, RegularUse);
+          readRegister(Reg, MI, RegularUse);
         Tracker.clobberRegister(Reg, *TRI, *TII, UseCopyInstr);
       }
 
@@ -941,7 +965,9 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
       if (!TRI->regsOverlap(RegDef, RegSrc)) {
         // Copy is now a candidate for deletion.
         MCRegister Def = RegDef.asMCReg();
-        if (!MRI->isReserved(Def))
+        // FIXME: what about src? is it target dependant? am I misunderstanding
+        // forward/backward prop here?
+        if (!isNeverRedundant(MI) && !isNeverRedundant(Def))
           MaybeDeadCopies.insert(&MI);
       }
     }
@@ -967,7 +993,7 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
           continue;
         }
       } else if (MO.readsReg())
-        ReadRegister(Reg.asMCReg(), MI, MO.isDebug() ? DebugUse : RegularUse);
+        readRegister(Reg.asMCReg(), MI, MO.isDebug() ? DebugUse : RegularUse);
     }
 
     // The instruction has a register mask operand which means that it clobbers
@@ -985,7 +1011,7 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
         std::optional<DestSourcePair> CopyOperands =
             isCopyInstr(*MaybeDead, *TII, UseCopyInstr);
         MCRegister Reg = CopyOperands->Destination->getReg().asMCReg();
-        assert(!MRI->isReserved(Reg));
+        assert(!isNeverRedundant(Reg));
 
         if (!RegMask->clobbersPhysReg(Reg)) {
           ++DI;
@@ -1056,7 +1082,7 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
 
       Register SrcReg = CopyOperands->Source->getReg();
       Register DestReg = CopyOperands->Destination->getReg();
-      assert(!MRI->isReserved(DestReg));
+      assert(!isNeverRedundant(DestReg));
 
       // Update matching debug values, if any.
       const auto &DbgUsers = CopyDbgUsers[MaybeDead];
@@ -1076,20 +1102,6 @@ void MachineCopyPropagation::ForwardCopyPropagateBlock(MachineBasicBlock &MBB) {
   Tracker.clear();
 }
 
-static bool isBackwardPropagatableCopy(const DestSourcePair &CopyOperands,
-                                       const MachineRegisterInfo &MRI) {
-  Register Def = CopyOperands.Destination->getReg();
-  Register Src = CopyOperands.Source->getReg();
-
-  if (!Def || !Src)
-    return false;
-
-  if (MRI.isReserved(Def) || MRI.isReserved(Src))
-    return false;
-
-  return CopyOperands.Source->isRenamable() && CopyOperands.Source->isKill();
-}
-
 void MachineCopyPropagation::propagateDefs(MachineInstr &MI) {
   if (!Tracker.hasAnyCopies())
     return;
@@ -1160,7 +1172,7 @@ void MachineCopyPropagation::propagateDefs(MachineInstr &MI) {
   }
 }
 
-void MachineCopyPropagation::BackwardCopyPropagateBlock(
+void MachineCopyPropagation::backwardCopyPropagateBlock(
     MachineBasicBlock &MBB) {
   LLVM_DEBUG(dbgs() << "MCP: BackwardCopyPropagateBlock " << MBB.getName()
                     << "\n");
@@ -1176,7 +1188,7 @@ void MachineCopyPropagation::BackwardCopyPropagateBlock(
       if (!TRI->regsOverlap(DefReg, SrcReg)) {
         // Unlike forward cp, we don't invoke propagateDefs here,
         // just let forward cp do COPY-to-COPY propagation.
-        if (isBackwardPropagatableCopy(*CopyOperands, *MRI)) {
+        if (isBackwardPropagatableCopy(MI, *CopyOperands)) {
           Tracker.invalidateRegister(SrcReg.asMCReg(), *TRI, *TII,
                                      UseCopyInstr);
           Tracker.invalidateRegister(DefReg.asMCReg(), *TRI, *TII,
@@ -1297,7 +1309,7 @@ void MachineCopyPropagation::BackwardCopyPropagateBlock(
 // instruction, we check registers in the operands of this instruction. If this
 // Reg is defined by a COPY, we untrack this Reg via
 // CopyTracker::clobberRegister(Reg, ...).
-void MachineCopyPropagation::EliminateSpillageCopies(MachineBasicBlock &MBB) {
+void MachineCopyPropagation::eliminateSpillageCopies(MachineBasicBlock &MBB) {
   // ChainLeader maps MI inside a spill-reload chain to its innermost reload COPY.
   // Thus we can track if a MI belongs to an existing spill-reload chain.
   DenseMap<MachineInstr *, MachineInstr *> ChainLeader;
@@ -1587,17 +1599,17 @@ MachineCopyPropagationPass::run(MachineFunction &MF,
 }
 
 bool MachineCopyPropagation::run(MachineFunction &MF) {
-  bool isSpillageCopyElimEnabled = false;
+  bool IsSpillageCopyElimEnabled = false;
   switch (EnableSpillageCopyElimination) {
   case cl::BOU_UNSET:
-    isSpillageCopyElimEnabled =
+    IsSpillageCopyElimEnabled =
         MF.getSubtarget().enableSpillageCopyElimination();
     break;
   case cl::BOU_TRUE:
-    isSpillageCopyElimEnabled = true;
+    IsSpillageCopyElimEnabled = true;
     break;
   case cl::BOU_FALSE:
-    isSpillageCopyElimEnabled = false;
+    IsSpillageCopyElimEnabled = false;
     break;
   }
 
@@ -1608,10 +1620,10 @@ bool MachineCopyPropagation::run(MachineFunction &MF) {
   MRI = &MF.getRegInfo();
 
   for (MachineBasicBlock &MBB : MF) {
-    if (isSpillageCopyElimEnabled)
-      EliminateSpillageCopies(MBB);
-    BackwardCopyPropagateBlock(MBB);
-    ForwardCopyPropagateBlock(MBB);
+    if (IsSpillageCopyElimEnabled)
+      eliminateSpillageCopies(MBB);
+    backwardCopyPropagateBlock(MBB);
+    forwardCopyPropagateBlock(MBB);
   }
 
   return Changed;

[s-barannikov]

A couple of nits, but I also find isNeverRedundant confusing, especially because it has several overloads and one of them unconditionally returns false.

[slinder1]

A couple of nits, but I also find isNeverRedundant confusing, especially because it has several overloads and one of them unconditionally returns false.

Sorry for the confusion around the unconditional overload, it is in preparation for #186237 later in the series.

[slinder1]

Changes since last push:

diff --git b/llvm/lib/CodeGen/MachineCopyPropagation.cpp a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -479,18 +479,19 @@ private:
                                           unsigned UseIdx);
   bool isBackwardPropagatableCopy(const MachineInstr &Copy,
                                   const DestSourcePair &CopyOperands);
+  /// Returns true iff a copy instruction having operand @p CopyOperand must
+  /// never be eliminated as redundant.
   bool isNeverRedundant(MCRegister CopyOperand) {
     // Avoid eliminating a copy from/to a reserved registers as we cannot
     // predict the value (Example: The sparc zero register is writable but stays
     // zero).
     return MRI->isReserved(CopyOperand);
   }
-  bool isNeverRedundant(const MachineInstr &Copy) { return false; }
-  bool isNeverRedundant(const MachineInstr &Copy,
-                        const DestSourcePair &CopyOperands) {
-    return isNeverRedundant(Copy) ||
-           isNeverRedundant(CopyOperands.Destination->getReg().asMCReg()) ||
-           isNeverRedundant(CopyOperands.Source->getReg().asMCReg());
+  /// Returns true iff the @p Copy instruction must never be eliminated as
+  /// redundant. This overload does not consider the operands of @p Copy.
+  bool isNeverRedundant(const MachineInstr &Copy) {
+    // FIXME: A future change will implement this.
+    return false;
   }
   bool hasImplicitOverlap(const MachineInstr &MI, const MachineOperand &Use);
   bool hasOverlappingMultipleDef(const MachineInstr &MI,
@@ -667,7 +668,7 @@ bool MachineCopyPropagation::isBackwardPropagatableCopy(
   if (!Def || !Src)
     return false;
 
-  if (isNeverRedundant(Copy, CopyOperands))
+  if (isNeverRedundant(Copy) || isNeverRedundant(Def) || isNeverRedundant(Src))
     return false;
 
   return CopyOperands.Source->isRenamable() && CopyOperands.Source->isKill();
@@ -965,8 +966,8 @@ void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
       if (!TRI->regsOverlap(RegDef, RegSrc)) {
         // Copy is now a candidate for deletion.
         MCRegister Def = RegDef.asMCReg();
-        // FIXME: what about src? is it target dependant? am I misunderstanding
-        // forward/backward prop here?
+        // FIXME: Document why this does not consider `RegSrc`, similar to how
+        // `backwardCopyPropagateBlock` does.
         if (!isNeverRedundant(MI) && !isNeverRedundant(Def))
           MaybeDeadCopies.insert(&MI);
       }
@@ -992,8 +993,9 @@ void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
           Defs.push_back(Reg.asMCReg());
           continue;
         }
-      } else if (MO.readsReg())
+      } else if (MO.readsReg()) {
         readRegister(Reg.asMCReg(), MI, MO.isDebug() ? DebugUse : RegularUse);
+      }
     }
 
     // The instruction has a register mask operand which means that it clobbers

[slinder1]

A couple of nits, but I also find isNeverRedundant confusing, especially because it has several overloads and one of them unconditionally returns false.

I removed the third overload, and documented the operand/instr versions. I am also open to other names. The negative phrasing isn't ideal, but the positive alternatives I came up with sound off: isEssential and isImportant

[slinder1]

Ping

[s-barannikov]

This pass requires NoVRegs property, so asMCReg() looks redundant. MachineVerifier would complain if there are any virtual registers, see MachineVerifier::verifyProperties().

[slinder1]

It only exists to assert that property, and the pass started with quite a lot of asMCReg. I can remove it all, but I would prefer slightly redundant asserts instead of missing any.

[slinder1]

Changes since last push:

diff --git b/llvm/lib/CodeGen/MachineCopyPropagation.cpp a/llvm/lib/CodeGen/MachineCopyPropagation.cpp
@@ -223,7 +223,8 @@ public:
           if (SrcCopy != Copies.end() && SrcCopy->second.LastSeenUseInCopy) {
             // If SrcCopy defines multiple values, we only need
             // to erase the record for Def in DefRegs.
-            for (auto *Itr = SrcCopy->second.DefRegs.begin();
+            // NOLINTNEXTLINE(llvm-qualified-auto)
+            for (auto Itr = SrcCopy->second.DefRegs.begin();
                  Itr != SrcCopy->second.DefRegs.end(); Itr++) {
               if (*Itr == Def) {
                 SrcCopy->second.DefRegs.erase(Itr);
@@ -487,12 +488,6 @@ private:
     // zero).
     return MRI->isReserved(CopyOperand);
   }
-  /// Returns true iff the @p Copy instruction must never be eliminated as
-  /// redundant. This overload does not consider the operands of @p Copy.
-  bool isNeverRedundant(const MachineInstr &Copy) {
-    // FIXME: A future change will implement this.
-    return false;
-  }
   bool hasImplicitOverlap(const MachineInstr &MI, const MachineOperand &Use);
   bool hasOverlappingMultipleDef(const MachineInstr &MI,
                                  const MachineOperand &MODef, MCRegister Def);
@@ -603,7 +598,7 @@ static bool isNopCopy(const MachineInstr &PreviousCopy, MCRegister Src,
 /// copying the super registers.
 bool MachineCopyPropagation::eraseIfRedundant(MachineInstr &Copy,
                                               MCRegister Src, MCRegister Def) {
-  if (isNeverRedundant(Copy) || isNeverRedundant(Src) || isNeverRedundant(Def))
+  if (isNeverRedundant(Src) || isNeverRedundant(Def))
     return false;
 
   // Search for an existing copy.
@@ -668,7 +663,7 @@ bool MachineCopyPropagation::isBackwardPropagatableCopy(
   if (!Def || !Src)
     return false;
 
-  if (isNeverRedundant(Copy) || isNeverRedundant(Def) || isNeverRedundant(Src))
+  if (isNeverRedundant(Def) || isNeverRedundant(Src))
     return false;
 
   return CopyOperands.Source->isRenamable() && CopyOperands.Source->isKill();
@@ -968,7 +963,7 @@ void MachineCopyPropagation::forwardCopyPropagateBlock(MachineBasicBlock &MBB) {
         MCRegister Def = RegDef.asMCReg();
         // FIXME: Document why this does not consider `RegSrc`, similar to how
         // `backwardCopyPropagateBlock` does.
-        if (!isNeverRedundant(MI) && !isNeverRedundant(Def))
+        if (!isNeverRedundant(Def))
           MaybeDeadCopies.insert(&MI);
       }
     }

[arsenm]

LGTM, some preexisting comments

[arsenm]

Don't understand the NOLINTNEXTLINE

[arsenm]

This example doesn't make sense (and is covered by isConstantPhysReg). Those writes are always redundant

[arsenm]

This is illegal?

[slinder1]

@s-barannikov I plan to merge this tomorrow, let me know if you still would like any changes and I can do them post-merge

[s-barannikov]

My concerns are non-blocking, thanks for heads-up

[slinder1]

🛠️ Changes since last push (click to expand):

[slinder1]

🛠️ Changes since last push (click to expand):