[PATCH] D109750: [GlobalISel][Legalizer] Don't use eraseFromParentAndMarkDBGValuesForRemoval() for some artifacts.

[Amara Emerson via Phabricator via llvm-commits]

aemerson created this revision.
aemerson added reviewers: paquette, arsenm, foad.
aemerson added a project: LLVM.
Herald added subscribers: kerbowa, hiraditya, kristof.beyls, rovka, nhaehnle, jvesely.
aemerson requested review of this revision.
Herald added a subscriber: wdng.

For artifacts excluding G_TRUNC/G_SEXT, which have IR counterparts, we don't
seem to have debug users of defs. However, in the legalizer we're always calling
MachineInstr::eraseFromParentAndMarkDBGValuesForRemoval() which is expensive.
In some rare cases, this contributes significantly to unreasonably long compile
times when we have lots of artifact combiner activity.

To verify this, I added asserts to that function when it actually replaced a debug
use operand with undef for these artifacts. On CTMark with both -O0 and -Os and
debug info enabled, I didn't see a single case where it triggered.

In my measurements I saw around a 0.5% geomean compile-time improvement on -g -O0
for AArch64 with this change.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D109750

Files:
  llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
  llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir


Index: llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir
===================================================================
--- llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir
+++ llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir
@@ -104,7 +104,7 @@
     ; GFX10: [[EXTRACT:%[0-9]+]]:_(s32) = G_EXTRACT [[LOAD]](<4 x s32>), 96
     ; GFX10: [[EXTRACT1:%[0-9]+]]:_(s32) = G_EXTRACT [[LOAD]](<4 x s32>), 64
     ; GFX10: [[DEF:%[0-9]+]]:_(s32) = G_IMPLICIT_DEF
-    ; GFX10: DBG_VALUE $noreg, $noreg, {{.*}}, !DIExpression(), debug-location !DILocation(line: 1, column: 1
+    ; GFX10: DBG_VALUE %dbg_use:_(s32), $noreg, <0x7fcfb8c4ad50>, !DIExpression(), debug-location !DILocation(line: 1, column: 1, scope: <0x7fcfb8c4adc0>)
     ; GFX10: G_STORE [[EXTRACT1]](s32), [[COPY]](p5) :: (store (s32), align 8, addrspace 5)
     ; GFX10: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 4
     ; GFX10: [[PTR_ADD:%[0-9]+]]:_(p5) = G_PTR_ADD [[COPY]], [[C]](s32)
Index: llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
===================================================================
--- llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
+++ llvm/lib/CodeGen/GlobalISel/Legalizer.cpp
@@ -171,6 +171,26 @@
     createdOrChangedInstr(MI);
   }
 };
+
+bool shouldSkipDbgValueFor(MachineInstr &MI) {
+  switch (MI.getOpcode()) {
+  case TargetOpcode::G_UNMERGE_VALUES:
+  case TargetOpcode::G_MERGE_VALUES:
+  case TargetOpcode::G_CONCAT_VECTORS:
+  case TargetOpcode::G_BUILD_VECTOR:
+  case TargetOpcode::G_EXTRACT:
+  case TargetOpcode::G_INSERT:
+    return true;
+  }
+  return false;
+}
+
+void eraseInstr(MachineInstr &MI) {
+  if (shouldSkipDbgValueFor(MI))
+    MI.eraseFromParent();
+  else
+    MI.eraseFromParentAndMarkDBGValuesForRemoval();
+}
 } // namespace
 
 Legalizer::MFResult
@@ -233,7 +253,7 @@
              "Expecting generic opcode");
       if (isTriviallyDead(MI, MRI)) {
         LLVM_DEBUG(dbgs() << MI << "Is dead; erasing.\n");
-        MI.eraseFromParentAndMarkDBGValuesForRemoval();
+        eraseInstr(MI);
         LocObserver.checkpoint(false);
         continue;
       }
@@ -283,7 +303,7 @@
       if (isTriviallyDead(MI, MRI)) {
         LLVM_DEBUG(dbgs() << MI << "Is dead\n");
         RemoveDeadInstFromLists(&MI);
-        MI.eraseFromParentAndMarkDBGValuesForRemoval();
+        eraseInstr(MI);
         LocObserver.checkpoint(false);
         continue;
       }
@@ -295,7 +315,7 @@
         for (auto *DeadMI : DeadInstructions) {
           LLVM_DEBUG(dbgs() << "Is dead: " << *DeadMI);
           RemoveDeadInstFromLists(DeadMI);
-          DeadMI->eraseFromParentAndMarkDBGValuesForRemoval();
+          eraseInstr(*DeadMI);
         }
         LocObserver.checkpoint(
             VerifyDebugLocs ==


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D109750.372445.patch
Type: text/x-patch
Size: 2820 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210914/c7db7ef4/attachment.bin>