[llvm] [CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead (PR #105956)

[via llvm-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-llvm-globalisel

Author: Tobias Stadler (tobias-stadler)

<details>
<summary>Changes</summary>

Merge GlobalISel's isTriviallyDead and DeadMachineInstructionElim's isDead code and move all unnecessary checks out of the hot path.

Functional changes to ponder over:
1. isTriviallyDead now never removes inlineasm like DeadMIElim.
2. We loop over the operands before doing any other checks. Please double-check if this ok for the loop in DeadMIElim.

See #<!-- -->105950 for why the LIFETIME markers check can't be moved to wouldBeTriviallyDead().

https://llvm-compile-time-tracker.com/compare.php?from=3e763db81607c71d0bb2eb4c01721ac6965d8de7&to=79267ac3120c3b54ecbdc490b079d72022669456&stat=instructions:u


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


4 Files Affected:

- (modified) llvm/include/llvm/CodeGen/MachineInstr.h (+4) 
- (modified) llvm/lib/CodeGen/DeadMachineInstructionElim.cpp (+5-18) 
- (modified) llvm/lib/CodeGen/GlobalISel/Utils.cpp (+10-19) 
- (modified) llvm/lib/CodeGen/MachineInstr.cpp (+21) 


``````````diff
diff --git a/llvm/include/llvm/CodeGen/MachineInstr.h b/llvm/include/llvm/CodeGen/MachineInstr.h
index 04c8144f2fe7af..42a4759510aa4c 100644
--- a/llvm/include/llvm/CodeGen/MachineInstr.h
+++ b/llvm/include/llvm/CodeGen/MachineInstr.h
@@ -1725,6 +1725,10 @@ class MachineInstr
   /// the instruction's location and its intended destination.
   bool isSafeToMove(bool &SawStore) const;
 
+  /// Return true if this instruction would be trivially dead if all its defined
+  /// registers were dead.
+  bool wouldBeTriviallyDead() const;
+
   /// Returns true if this instruction's memory access aliases the memory
   /// access of Other.
   //
diff --git a/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp b/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
index 7fc25cd889a0df..6f87a975797f15 100644
--- a/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
+++ b/llvm/lib/CodeGen/DeadMachineInstructionElim.cpp
@@ -80,22 +80,9 @@ INITIALIZE_PASS(DeadMachineInstructionElim, DEBUG_TYPE,
                 "Remove dead machine instructions", false, false)
 
 bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
-  // Technically speaking inline asm without side effects and no defs can still
-  // be deleted. But there is so much bad inline asm code out there, we should
-  // let them be.
-  if (MI->isInlineAsm())
-    return false;
-
-  // Don't delete frame allocation labels.
-  if (MI->getOpcode() == TargetOpcode::LOCAL_ESCAPE)
-    return false;
-
-  // Don't delete instructions with side effects.
-  bool SawStore = false;
-  if (!MI->isSafeToMove(SawStore) && !MI->isPHI())
-    return false;
-
-  // Examine each operand.
+  // Instructions without side-effects are dead iff they only define dead regs.
+  // This function is very hot and this loop returns early in the common case,
+  // so only perform additional checks before this if absolutely necessary.
   for (const MachineOperand &MO : MI->all_defs()) {
     Register Reg = MO.getReg();
     if (Reg.isPhysical()) {
@@ -119,8 +106,8 @@ bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
     }
   }
 
-  // If there are no defs with uses, the instruction is dead.
-  return true;
+  // If there are no defs with uses, the instruction might be dead.
+  return MI->wouldBeTriviallyDead();
 }
 
 bool DeadMachineInstructionElimImpl::runImpl(MachineFunction &MF) {
diff --git a/llvm/lib/CodeGen/GlobalISel/Utils.cpp b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
index cfdd9905c16fa6..4b8ede6dc7fd9b 100644
--- a/llvm/lib/CodeGen/GlobalISel/Utils.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/Utils.cpp
@@ -221,31 +221,22 @@ bool llvm::canReplaceReg(Register DstReg, Register SrcReg,
 
 bool llvm::isTriviallyDead(const MachineInstr &MI,
                            const MachineRegisterInfo &MRI) {
-  // FIXME: This logical is mostly duplicated with
-  // DeadMachineInstructionElim::isDead. Why is LOCAL_ESCAPE not considered in
-  // MachineInstr::isLabel?
+  // Instructions without side-effects are dead iff they only define dead regs.
+  // This function is very hot and this loop returns early in the common case,
+  // so only perform additional checks before this if absolutely necessary.
+  for (const auto &MO : MI.all_defs()) {
+    Register Reg = MO.getReg();
+    if (Reg.isPhysical() || !MRI.use_nodbg_empty(Reg))
+      return false;
+  }
 
-  // Don't delete frame allocation labels.
-  if (MI.getOpcode() == TargetOpcode::LOCAL_ESCAPE)
-    return false;
   // LIFETIME markers should be preserved even if they seem dead.
+  // FIXME: See issue #105950 for why this isn't in wouldBeTriviallyDead().
   if (MI.getOpcode() == TargetOpcode::LIFETIME_START ||
       MI.getOpcode() == TargetOpcode::LIFETIME_END)
     return false;
 
-  // If we can move an instruction, we can remove it.  Otherwise, it has
-  // a side-effect of some sort.
-  bool SawStore = false;
-  if (!MI.isSafeToMove(SawStore) && !MI.isPHI())
-    return false;
-
-  // Instructions without side-effects are dead iff they only define dead vregs.
-  for (const auto &MO : MI.all_defs()) {
-    Register Reg = MO.getReg();
-    if (Reg.isPhysical() || !MRI.use_nodbg_empty(Reg))
-      return false;
-  }
-  return true;
+  return MI.wouldBeTriviallyDead();
 }
 
 static void reportGISelDiagnostic(DiagnosticSeverity Severity,
diff --git a/llvm/lib/CodeGen/MachineInstr.cpp b/llvm/lib/CodeGen/MachineInstr.cpp
index 0f2acdb12389d4..f5894b98614427 100644
--- a/llvm/lib/CodeGen/MachineInstr.cpp
+++ b/llvm/lib/CodeGen/MachineInstr.cpp
@@ -1323,6 +1323,27 @@ bool MachineInstr::isSafeToMove(bool &SawStore) const {
   return true;
 }
 
+bool MachineInstr::wouldBeTriviallyDead() const {
+  // Technically speaking inline asm without side effects and no defs can still
+  // be deleted. But there is so much bad inline asm code out there, we should
+  // let them be.
+  if (isInlineAsm())
+    return false;
+
+  // Don't delete frame allocation labels.
+  // FIXME: Why is LOCAL_ESCAPE not considered in MachineInstr::isLabel?
+  if (getOpcode() == TargetOpcode::LOCAL_ESCAPE)
+    return false;
+
+  // If we can move an instruction, we can remove it.  Otherwise, it has
+  // a side-effect of some sort.
+  bool SawStore = false;
+  if (!isSafeToMove(SawStore) && !isPHI())
+    return false;
+
+  return true;
+}
+
 static bool MemOperandsHaveAlias(const MachineFrameInfo &MFI, AAResults *AA,
                                  bool UseTBAA, const MachineMemOperand *MMOa,
                                  const MachineMemOperand *MMOb) {

``````````

</details>


https://github.com/llvm/llvm-project/pull/105956