[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

tobias-stadler · 2024-08-24T19:59:45Z

Merge GlobalISel's isTriviallyDead and DeadMachineInstructionElim's isDead code and remove all unnecessary checks from the hot path by looping over the operands before doing any other checks. Please double-check if this ok for the loop in DeadMIElim.

See #105950 for why DeadMIElim needs to remove LIFETIME markers even though they probably shouldn't generally be considered dead.

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

llvmbot · 2024-08-24T20:00:19Z

@llvm/pr-subscribers-llvm-globalisel

Author: Tobias Stadler (tobias-stadler)

Changes

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

Functional changes to ponder over:

isTriviallyDead now never removes inlineasm like DeadMIElim.
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 --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) {

tobias-stadler · 2024-08-24T20:03:13Z

AArch64 GlobalISel CTMark
O0:

Program                                       compile_instructions                        
                                              base-O0              patch-O0        diff   
7zip/7zip-benchmark                           142579160298.00      142443007662.00 -0.10% 
Bullet/bullet                                  61367419175.00       61242533522.00 -0.20% 
kimwitu++/kc                                   22466504799.00       22410469877.00 -0.25% 
SPASS/SPASS                                    14326291495.00       14262677945.00 -0.44% 
ClamAV/clamscan                                14145778197.00       14068220767.00 -0.55% 
tramp3d-v4/tramp3d-v4                          18327257117.00       18225535659.00 -0.56% 
mafft/pairlocalalign                            6717269770.00        6669638138.00 -0.71% 
lencod/lencod                                  12283466464.00       12179009286.00 -0.85% 
consumer-typeset/consumer-typeset              11762311006.00       11656647438.00 -0.90% 
sqlite3/sqlite3                                 4369543098.00        4314547563.00 -1.26% 
                           Geomean difference                                      -0.58%

O2:

Program                                       compile_instructions                       
                                              base-O2              patch-O2        diff  
7zip/7zip-benchmark                           205561068160.00      205477950143.00 -0.04%
Bullet/bullet                                  99533309415.00       99444270943.00 -0.09%
SPASS/SPASS                                    42161673055.00       42091709441.00 -0.17%
ClamAV/clamscan                                52108752865.00       52020947264.00 -0.17%
tramp3d-v4/tramp3d-v4                          64953199075.00       64822894336.00 -0.20%
mafft/pairlocalalign                           31775234940.00       31708063069.00 -0.21%
lencod/lencod                                  55545156416.00       55417527751.00 -0.23%
consumer-typeset/consumer-typeset              34402637417.00       34323251919.00 -0.23%
kimwitu++/kc                                   40736268734.00       40628351259.00 -0.26%
sqlite3/sqlite3                                33818134349.00       33728194647.00 -0.27%
                           Geomean difference                                      -0.19%

llvm/include/llvm/CodeGen/MachineInstr.h

nikic · 2024-08-27T11:03:35Z

llvm/lib/CodeGen/DeadMachineInstructionElim.cpp

+    return false;
+
+  // FIXME: See issue #105950 for why LIFETIME markers are considered dead here.
+  if (MI->isLifetimeMarker())


Wonder why lifetime markers are considered safe to move...

It seems like lifetime markers should be considered in MachineInstr::isPosition, which would also make them unsafe to move.
I might try moving the LIFETIME marker and LOCAL_ESCAPE checks in separate commits... to make this easy to revert when it inevitably breaks someone's code :).

arsenm · 2024-08-28T13:22:02Z

llvm/lib/CodeGen/DeadMachineInstructionElim.cpp

+    return false;
+
+  // FIXME: See issue #105950 for why LIFETIME markers are considered dead here.
+  if (MI->isLifetimeMarker())


I don't like considering isLifetimeMarker here and the opposite down in wouldBeTriviallyDead but I guess you're planning on fixing that

Yes, I want to fix this properly, but I wanted to keep this NFC except for the compile-time improvements. I think for now it's better to make this sketchy behavior explicit in DeadMIElim rather than silently dropping lifetime markers by pretending they are safe to move, which makes little sense. I want to add lifetime markers to MachineInstr::isPosition, which makes them unsafe to move. This should get rid of the ugly opposite check.

arsenm · 2024-08-28T13:22:59Z

llvm/lib/CodeGen/DeadMachineInstructionElim.cpp

@@ -119,8 +106,18 @@ bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
    }


Why doesn't the isPhysical path consider the dead flag on the operand?

arsenm · 2024-08-28T17:10:41Z

llvm/lib/CodeGen/MachineInstr.cpp

+  if (!isSafeToMove(SawStore) && !isPHI())
+    return false;
+
+  return true;


fold to return of bool expression?

arsenm · 2024-08-29T08:32:07Z

llvm/lib/CodeGen/MachineInstr.cpp

-    return false;
-
-  return true;
+  return isSafeToMove(SawStore) || isPHI();


isPHI first

tobias-stadler · 2024-08-29T20:04:36Z

Thanks for the reviews.

Conflicts are due to introduction of FAKE_USE. This adds another edgecase to isDead, which I am not sure should really be there considering the other properties the instruction is supposed to have. I have raised this here: #86149 (comment). Waiting with fixing the conflicts for now.

tobias-stadler requested review from aemerson, arsenm and nikic August 24, 2024 19:59

llvmbot added the llvm:globalisel label Aug 24, 2024

tobias-stadler force-pushed the perf/dead branch from 79267ac to 0f4e84a Compare August 25, 2024 21:30

nikic reviewed Aug 27, 2024

View reviewed changes

arsenm reviewed Aug 28, 2024

View reviewed changes

arsenm approved these changes Aug 28, 2024

View reviewed changes

arsenm reviewed Aug 28, 2024

View reviewed changes

arsenm reviewed Aug 29, 2024

View reviewed changes

llvm/lib/CodeGen/MachineInstr.cpp Outdated

return false;

return true;

return isSafeToMove(SawStore) || isPHI();

Copy link

Contributor

arsenm Aug 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isPHI first

arsenm approved these changes Aug 29, 2024

View reviewed changes

tobias-stadler added 4 commits September 9, 2024 14:38

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead

e19d7be

Rename function

40d27dc

Return expression

6aaf161

phi check first

aff8bd2

tobias-stadler force-pushed the perf/dead branch from 5cf2459 to aff8bd2 Compare September 9, 2024 13:19

tobias-stadler merged commit 2d338be into llvm:main Sep 9, 2024
8 checks passed

tobias-stadler deleted the perf/dead branch September 9, 2024 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

Uh oh!

tobias-stadler commented Aug 24, 2024 •

edited

Loading

Uh oh!

llvmbot commented Aug 24, 2024

Uh oh!

tobias-stadler commented Aug 24, 2024

Uh oh!

Uh oh!

nikic Aug 27, 2024

Uh oh!

tobias-stadler Aug 27, 2024

Uh oh!

arsenm Aug 28, 2024

Uh oh!

tobias-stadler Aug 28, 2024 •

edited

Loading

Uh oh!

arsenm Aug 28, 2024

Uh oh!

arsenm Aug 28, 2024

Uh oh!

arsenm Aug 29, 2024

Uh oh!

tobias-stadler commented Aug 29, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

		@@ -119,8 +106,18 @@ bool DeadMachineInstructionElimImpl::isDead(const MachineInstr *MI) const {
		}

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

[CodeGen] Refactor DeadMIElim isDead and GISel isTriviallyDead #105956

Uh oh!

Conversation

tobias-stadler commented Aug 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Aug 24, 2024

Uh oh!

tobias-stadler commented Aug 24, 2024

Uh oh!

Uh oh!

nikic Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

tobias-stadler Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

tobias-stadler Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arsenm Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

tobias-stadler commented Aug 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tobias-stadler commented Aug 24, 2024 •

edited

Loading

tobias-stadler Aug 28, 2024 •

edited

Loading

tobias-stadler commented Aug 29, 2024 •

edited

Loading