[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

4597

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

4597

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

2–3

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.

21–22

[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

21–22

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

Full marks for writing good test explanations

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/DebugCounter.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/KnownBits.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Transforms/InstCombine/InstCombine.h"
+#include "llvm/Transforms/Utils/BasicBlockUtils.h"
 #include "llvm/Transforms/Utils/Local.h"
 #include <algorithm>
 #include <cassert>
 #include <cstdint>
 #include <memory>
 #include <string>
 #include <utility>
 
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/do-not-remove-redundant-dbg.ll

+; RUN: opt -passes=instcombine -S %s -o -  \
+; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg"
+
+;; Based on the test remove-redundant-dbg.ll.
+;;
+;; Check that instcombine does not remove redundant debug intrinsics when
+;; assignment tracking is turned off. This test is here to check the existing
+;; behaviour is maintained. If it is discovered that it is profitable to remove
+;; these intrinsics in instcombine then it's okay to remove this test.
+
+; CHECK: @llvm.dbg.value(metadata i32 undef
+; CHECK: @llvm.dbg.value(metadata i32 0
+; CHECK: @llvm.dbg.value(metadata i32 1
+
+define dso_local void @_Z3funv() local_unnamed_addr !dbg !7 {
+entry:
+  call void @llvm.dbg.value(metadata i32 undef, metadata !11, metadata !DIExpression()), !dbg !14
+  call void @_Z3extv(), !dbg !15
+  call void @llvm.dbg.value(metadata i32 0, metadata !11, metadata !DIExpression()), !dbg !14
+  call void @llvm.dbg.value(metadata i32 1, metadata !11, metadata !DIExpression()), !dbg !14
+  ret void, !dbg !16
+}
+
+declare !dbg !17 dso_local void @_Z3extv() local_unnamed_addr
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!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 = !{}

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 ptr 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 ptr undef, metadata !DIExpression()), !dbg !14
+  call void @llvm.dbg.assign(metadata i32 1, metadata !11, metadata !DIExpression(), metadata !13, metadata ptr undef, metadata !DIExpression()), !dbg !14
+  ret void, !dbg !16
+}
+
+declare !dbg !17 dso_local void @_Z3extv() local_unnamed_addr
+declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
+
+!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 = !{}

llvm/test/DebugInfo/Generic/assignment-tracking/salvage-value.ll

 ; RUN: opt %s -S -o - -passes=instcombine -experimental-assignment-tracking \
 ; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg"
 
 ;; Hand-written (the debug info doesn't necessarily make sense and isn't fully
 ;; formed). Test salvaging a dbg.assign value and address. Checks and comments
 ;; are inline.
 
 define dso_local void @fun(i32  %x, i32 %y, ptr %p) !dbg !7 {
 entry:
   %add = add nsw i32 %x, 1, !dbg !22
   call void @llvm.dbg.assign(metadata i32 %add, metadata !14, metadata !DIExpression(), metadata !28, metadata ptr %p, metadata !DIExpression()), !dbg !16
 ;; %add is salvaged.
 ; CHECK: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}metadata !DIExpression(DW_OP_plus_uconst, 1, DW_OP_stack_value),{{.+}}, metadata ptr %p, metadata !DIExpression())
 
  %add1 = add nsw i32 %x, %y, !dbg !29
- call void @llvm.dbg.assign(metadata i32 %add1, metadata !14, metadata !DIExpression(), metadata !31, metadata ptr %p, metadata !DIExpression()), !dbg !16
+ call void @llvm.dbg.assign(metadata i32 %add1, metadata !32, metadata !DIExpression(), metadata !31, metadata ptr %p, metadata !DIExpression()), !dbg !16
 ;; %add1 is not salvaged as it requires two values and DIArgList is
 ;; not (yet) supported for dbg.assigns.
 ; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 undef,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr %p, metadata !DIExpression())
 
   %arrayidx0 = getelementptr inbounds i32, ptr %p, i32 0
   call void @llvm.dbg.assign(metadata i32 %x, metadata !14, metadata !DIExpression(), metadata !17, metadata ptr %arrayidx0, metadata !DIExpression()), !dbg !16
 ;; %arrayidx0 is salvaged (zero offset, so the gep is just replaced with %p).
 ; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr %p, metadata !DIExpression())
 
   %arrayidx1 = getelementptr inbounds i32, ptr %p, i32 1
-  call void @llvm.dbg.assign(metadata i32 %x, metadata !14, metadata !DIExpression(), metadata !18, metadata ptr %arrayidx1, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.assign(metadata i32 %x, metadata !33, metadata !DIExpression(), metadata !18, metadata ptr %arrayidx1, metadata !DIExpression()), !dbg !16
 ;; %arrayidx1 is salvaged.
 ; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr %p, metadata !DIExpression(DW_OP_plus_uconst, 4))
 
   %arrayidx2 = getelementptr inbounds i32, ptr %p, i32 %x
-  call void @llvm.dbg.assign(metadata i32 %x, metadata !14, metadata !DIExpression(), metadata !19, metadata ptr %arrayidx2, metadata !DIExpression()), !dbg !16
+  call void @llvm.dbg.assign(metadata i32 %x, metadata !34, metadata !DIExpression(), metadata !19, metadata ptr %arrayidx2, metadata !DIExpression()), !dbg !16
 ;; Variadic DIExpressions for dbg.assign address component is not supported -
 ;; set undef.
 ; CHECK-NEXT: call void @llvm.dbg.assign(metadata i32 %x,{{.+}}, metadata !DIExpression(),{{.+}}, metadata ptr undef, metadata !DIExpression())
 
   ret void
 }
 
 declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)

 !17 = distinct !DIAssignID()
 !18 = distinct !DIAssignID()
 !19 = distinct !DIAssignID()
 !21 = !DILocation(line: 3, column: 3, scope: !7)
 !22 = !DILocation(line: 3, column: 17, scope: !7)
 !28 = distinct !DIAssignID()
 !29 = !DILocation(line: 4, column: 13, scope: !7)
 !31 = distinct !DIAssignID()
+!32 = !DILocalVariable(name: "Local1", scope: !7, file: !1, line: 3, type: !10)
+!33 = !DILocalVariable(name: "Local2", scope: !7, file: !1, line: 3, type: !10)
+!34 = !DILocalVariable(name: "Local3", scope: !7, file: !1, line: 3, type: !10)
+!35 = !DILocalVariable(name: "Local4", scope: !7, file: !1, line: 3, type: !10)