[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

4353

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

4353

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 ↗

(On Diff #460361)

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 ↗

(On Diff #460361)

[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 ↗

(On Diff #460361)

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) {