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

[aemerson]

Summary

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.

[aemerson]

Fix test.

[arsenm]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

175 ↗

(On Diff #372446)

Don't we already have an isArtifact check somewhere?

[arsenm]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

175 ↗

(On Diff #372446)

Oh, I just reread and this is different. Can you add a comment justifying why these operations are skipped?

[arsenm]

Maybe should add a verifier check that these never have dbg users

[Petar.Avramovic]

llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir

87

This is now broken, dbg_use has no def. Maybe delete this test since it was artificial to begin with. Relevant tests are above.

[aemerson]

In D109750#2999516, @arsenm wrote:

Maybe should add a verifier check that these never have dbg users

I don't think we should go that far. If we do have a debug user then not marking the use as undef AFAICT isn't strictly incorrect.

[aemerson]

Add comment.

[arsenm]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

185 ↗

(On Diff #372817)

Should still get a comment explaining why these operations

[aemerson]

llvm/lib/CodeGen/GlobalISel/Legalizer.cpp

185 ↗

(On Diff #372817)

I updated very shortly before your reply. Is this comment ok?

[foad]

Needs rebasing (maybe re-benchmarking too?) after D109154.

[aemerson]

In D109750#3009146, @foad wrote:

Needs rebasing (maybe re-benchmarking too?) after D109154.

I think this is orthogonal to D109154 so don't need to rebenchmark.

[aemerson]

Rebase

[paquette]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1060

might as well pull the return false; into the default case?

[arsenm]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1060

I think the only combination that stops warnings on all compilers is return false in the default case with an llvm_unreachable after the switch

[dsanders]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1054–1058

I don't think this assumption holds beyond the first time the legalizer processes each instruction and is dubious earlier than that if you have pre-legalization passes too.

For the standard pipeline of passes, suppose you have an expansion in the legalizer that replaces some operation with a DBG_VALUE with something ending in G_MERGE_VALUES (or one of the others). On the next legalization step you have a G_MERGE_VALUES with a DBG_VALUE. That already breaks the assumption it doesn't have one but it takes a bit more to go wrong. If anything causes this G_MERGE_VALUES to be deleted without replacement you are left with a use-without-def as the DBG_VALUE is not erased. For example, if this is one lane of a bigger G_UNMERGE_VALUES/G_MERGE_VALUES pair and something proves the lane isn't needed it would erase all the instructions for the lane but leave the DBG_VALUE behind.

I haven't dug into it myself but I'm told we've run into this scenario in our downstream target and we've had to revert this change.

[jackoalan]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1054–1058

I'd like to echo this claim. It is far too presumptuous to assume artifacts like G_MERGE_VALUES are never created pre-legalizer for all targets. If anything, this decision should be made per-target in LegalizerInfo.

[jackoalan]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

1054–1058

I found a specific case where GlobalISel's IRTranslator emits G_MERGE_VALUES: Lowering a call with arguments that are wider than native machine types. I think G_UNMERGE_VALUES, G_MERGE_VALUES, G_CONCAT_VECTORS, G_BUILD_VECTOR should be excluded from this test because they are all reachable in CallLowering and therefore originate from IR.

llvm/lib/CodeGen/GlobalISel/Utils.cpp

 
 bool llvm::shouldOptForSize(const MachineBasicBlock &MBB,
                             ProfileSummaryInfo *PSI, BlockFrequencyInfo *BFI) {
   const auto &F = MBB.getParent()->getFunction();
   return F.hasOptSize() || F.hasMinSize() ||
          llvm::shouldOptimizeForSize(MBB.getBasicBlock(), PSI, BFI);
 }
 
+/// These artifacts generally don't have any debug users because they don't
+/// directly originate from IR instructions, but instead usually from
+/// legalization. Avoiding checking for debug users improves compile time.
+/// Note that truncates or extends aren't included because they have IR
+/// counterparts which can have debug users after translation.
+static 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;
+  default:
+    return false;
+  }
+}
+
 void llvm::saveUsesAndErase(MachineInstr &MI, MachineRegisterInfo &MRI,
                             LostDebugLocObserver *LocObserver,
                             SmallInstListTy &DeadInstChain) {
   for (MachineOperand &Op : MI.uses()) {
     if (Op.isReg() && Op.getReg().isVirtual())
       DeadInstChain.insert(MRI.getVRegDef(Op.getReg()));
   }
   LLVM_DEBUG(dbgs() << MI << "Is dead; erasing.\n");
   DeadInstChain.remove(&MI);
+  if (shouldSkipDbgValueFor(MI))
+    MI.eraseFromParent();
+  else
     MI.eraseFromParentAndMarkDBGValuesForRemoval();
   if (LocObserver)
     LocObserver->checkpoint(false);
 }
 
 void llvm::eraseInstrs(ArrayRef<MachineInstr *> DeadInstrs,
                        MachineRegisterInfo &MRI,
                        LostDebugLocObserver *LocObserver) {
   SmallInstListTy DeadInstChain;

llvm/test/CodeGen/AMDGPU/GlobalISel/bug-legalization-artifact-combiner-dead-def.mir

 # NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
 # RUN: llc -mtriple=amdgcn-mesa-mesa3d -mcpu=gfx1010 -O0 -run-pass=legalizer %s -o - | FileCheck %s --check-prefix=GFX10
 
 --- |
 
   define void @value_finder_bug() { ret void }
   define void @value_finder_bug_before_artifact_combine() { ret void }
-  define void @value_finder_bug_before_artifact_combine_dbg_use() { ret void }
 
   !0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "llvm", isOptimized: false, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
   !1 = !DIFile(filename: "bug-legalization-artifact-combiner-dead-def", directory: "/tmp")
   !2 = !{}
   !3 = !{i32 2, !"Dwarf Version", i32 4}
   !4 = !{i32 2, !"Debug Info Version", i32 3}
   !5 = distinct !DISubprogram(name: "value_finder_bug_before_artifact_combine_dbg_use", scope: !1, file: !1, line: 1, type: !6, isLocal: false, isDefinition: true, scopeLine: 1, flags: DIFlagPrototyped, isOptimized: false, unit: !0, retainedNodes: !2)
   !6 = !DISubroutineType(types: !2)
bb.0:
     %8:_(<2 x s32>) = G_BUILD_VECTOR %6(s32), %7(s32)
     %9:_(<2 x s32>) = G_INSERT %8, %5(s32), 32
     %deaf_def:_(s32), %11:_(s32) = G_UNMERGE_VALUES %9(<2 x s32>)
     G_STORE %6(s32), %0(p5) :: (store (s32), align 8, addrspace 5)
     %12:_(s32) = G_CONSTANT i32 4
     %13:_(p5) = G_PTR_ADD %0, %12(s32)
     G_STORE %11(s32), %13(p5) :: (store (s32) into unknown-address + 4, addrspace 5)
 
 ...
-
----
-name: value_finder_bug_before_artifact_combine_dbg_use
-body: |
-  bb.0:
-    liveins: $vgpr0, $vgpr1, $vgpr2
-
-    ; GFX10-LABEL: name: value_finder_bug_before_artifact_combine_dbg_use
-    ; GFX10: [[COPY:%[0-9]+]]:_(p5) = COPY $vgpr0
-    ; GFX10: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
-    ; GFX10: [[COPY2:%[0-9]+]]:_(s32) = COPY $vgpr2
-    ; GFX10: [[MV:%[0-9]+]]:_(p4) = G_MERGE_VALUES [[COPY1]](s32), [[COPY2]](s32)
-    ; GFX10: [[LOAD:%[0-9]+]]:_(<4 x s32>) = G_LOAD [[MV]](p4) :: (load (<4 x s32>), align 4, addrspace 4)
-    ; 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: DBG_VALUE $noreg, $noreg, {{.*}}, !DIExpression(), debug-location !DILocation(line: 1, column: 1
-    ; 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)
-    ; GFX10: G_STORE [[EXTRACT]](s32), [[PTR_ADD]](p5) :: (store (s32) into unknown-address + 4, addrspace 5)
-    %0:_(p5) = COPY $vgpr0
-    %1:_(s32) = COPY $vgpr1
-    %2:_(s32) = COPY $vgpr2
-    %3:_(p4) = G_MERGE_VALUES %1(s32), %2(s32)
-    %4:_(<4 x s32>) = G_LOAD %3(p4) :: (load (<4 x s32>), align 4, addrspace 4)
-    %5:_(s32) = G_EXTRACT %4(<4 x s32>), 96
-    %6:_(s32) = G_EXTRACT %4(<4 x s32>), 64
-    %7:_(s32) = G_IMPLICIT_DEF
-    %8:_(<2 x s32>) = G_BUILD_VECTOR %6(s32), %7(s32)
-    %9:_(<2 x s32>) = G_INSERT %8, %5(s32), 32
-    %dbg_use:_(s32), %11:_(s32) = G_UNMERGE_VALUES %9(<2 x s32>)
-    DBG_VALUE %dbg_use(s32), $noreg, !7, !DIExpression(), debug-location !9
-    G_STORE %6(s32), %0(p5) :: (store (s32), align 8, addrspace 5)
-    %12:_(s32) = G_CONSTANT i32 4
-    %13:_(p5) = G_PTR_ADD %0, %12(s32)
-    G_STORE %11(s32), %13(p5) :: (store (s32) into unknown-address + 4, addrspace 5)
-...