[Assignment Tracking][24/*] Always RemoveRedundantDbgInstrs in instcombine in assignment tracking builds

[Orlando]

Summary

This reduces peak memory overhead by 15% when building CTMark's tramp3d-v4 with -O2 -g with assignment tracking enabled.

---

When building CTMark's tramp3d-v4 (with -emit-llvm and RelWithDebInfo flags) with this patch stack the peak memory consumption increases by just under 1.5% when assignment tracking is enabled, and the run time difference (of the compiler) is negligible.

[chrisjackson]

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

4352

Possible suitable branch hint location, based on your comment?

I'm not sure they are in the llvm's c++ version though.

[jmorse]

LGTM; but annoying as this is, a test for the behaviour is desirable, because we want to keep the behaviour in the future and someone refactoring the area might inadvertently regress memory usage. (15% is pretty big, so the behaviour is important).

[Orlando]

+ Add test

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

4352

This path is only hit once per instcombine run - IMHO it's not hot enough to worry about that. FWIW The hot areas in this patch stack are all around DebugVariable hashing and comparison in DenseMap and increased work/calls in/of RemoveRedundantDbgInstrs (we still do need this patch until deeper performance work takes place IMO).

[StephenTozer]

Minor comments, but this looks generally good.

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/remove-redundant-dbg.ll

1–2

Might be overthinking it, but could there also be a test that we don't remove redundant debug intrinsics when experimental assignment tracking is off? It's not really an error if those intrinsics were removed, but since we're not choosing to enable it for all builds (presumably for performance reasons) that should probably be covered in this test.

20–21

[Orlando]

+ rebase
+ add test remove-redundant-dbg.ll to check existing behaviour is maintained without assignment tracking
+ modify a salvaging test that uses instcombine so that dbg intrinsics are not removed

[Orlando]

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/remove-redundant-dbg.ll

20–21

Oops - did this in the new test but not here. Note to self: do this before committing.

[jmorse]

LGTM

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/do-not-remove-redundant-dbg.ll

6–9 ↗

(On Diff #475716)

Full marks for writing good test explanations

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

IRBuilder<TargetFolder, IRBuilderCallbackInserter> Builder(
           AC.registerAssumption(Assume);
       }));
 
   // Lower dbg.declare intrinsics otherwise their value may be clobbered
   // by instcombiner.
   bool MadeIRChange = false;
   if (ShouldLowerDbgDeclare)
     MadeIRChange = LowerDbgDeclare(F);
+  // LowerDbgDeclare calls RemoveRedundantDbgInstrs, but LowerDbgDeclare will
+  // almost never return true when running an assignment tracking build. Take
+  // this opportunity to do some clean up for assignment tracking builds too.
+  if (!MadeIRChange && getEnableAssignmentTracking()) {
+    for (auto &BB : F)
+      RemoveRedundantDbgInstrs(&BB);
+  }
 
   // Iterate while there is work to do.
   unsigned Iteration = 0;
   while (true) {
     ++NumWorklistIterations;
     ++Iteration;
 
     if (Iteration > InfiniteLoopDetectionThreshold) {

llvm/test/DebugInfo/Generic/assignment-tracking/instcombine/remove-redundant-dbg.ll

+; RUN: opt -passes=instcombine -S %s -o - -experimental-assignment-tracking \
+; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg"
+
+;; Check that instcombine removes redundant debug intrinsics. This has a
+;; significant positive impact on peak memory. CTMark's tramp3d-v4's peak
+;; memory with assignment tracking enabled is reduced by about 15% with this
+;; change.
+
+; CHECK: @llvm.dbg.assign(metadata i32 1
+
+define dso_local void @_Z3funv() local_unnamed_addr !dbg !7 {
+entry:
+   call void @llvm.dbg.assign(metadata i32 undef, metadata !11, metadata !DIExpression(), metadata !13, metadata i32* undef, metadata !DIExpression()), !dbg !14
+   call void @_Z3extv(), !dbg !15
+  call void @llvm.dbg.assign(metadata i32 0, metadata !11, metadata !DIExpression(), metadata !13, metadata i32* undef, metadata !DIExpression()), !dbg !14
+  call void @llvm.dbg.assign(metadata i32 1, metadata !11, metadata !DIExpression(), metadata !13, metadata i32* undef, metadata !DIExpression()), !dbg !14
+  ret void, !dbg !16
+}
+
+declare !dbg !17 dso_local void @_Z3extv() local_unnamed_addr #1
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata) #2
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus_14, file: !1, producer: "clang version 14.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "test.cpp", directory: "/")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"uwtable", i32 1}
+!6 = !{!"clang version 14.0.0)"}
+!7 = distinct !DISubprogram(name: "fun", linkageName: "_Z3funv", scope: !1, file: !1, line: 2, type: !8, scopeLine: 2, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+!8 = !DISubroutineType(types: !9)
+!9 = !{null}
+!10 = !{!11}
+!11 = !DILocalVariable(name: "a", scope: !7, file: !1, line: 2, type: !12)
+!12 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!13 = distinct !DIAssignID()
+!14 = !DILocation(line: 0, scope: !7)
+!15 = !DILocation(line: 2, column: 25, scope: !7)
+!16 = !DILocation(line: 2, column: 32, scope: !7)
+!17 = !DISubprogram(name: "ext", linkageName: "_Z3extv", scope: !1, file: !1, line: 1, type: !8, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !18)
+!18 = !{}