[DebugInfo][RemoveDIs] Final omnibus test fixing for RemoveDIs

[jmorse]

With this, I get a clean test suite running under RemoveDIs, the non-intrinsic representation of debug-info, including under asan. We've previously established that we generate identical binaries for some large projects, so this i just edge-case cleanup. The changes:

• CodeGenPrepare fixups need to apply to dbg.assigns as well as dbg.values (a dbg.assign is a dbg.value).
• Pin a test for constant-deletion to intrinsic debug-info: this very rare scenario uses a different kill-location sigil in dbg.value mode to RemoveDIs mode, which generates spurious test differences.
• Suppress a memory leak in a unit test: the code for dealing with trailing debug-info in a block is necessarily fiddly, leading to this leak when testing it. Developer-facing interfaces for moving instructions around always deal with this behind the scenes.
• SROA, when replacing some vector-loads, needs to insert the replacement loads ahead of any debug-info records so that their values remain dominated by a definition. Set the head-bit indicating our insertion should come before debug-info.

[llvmbot]

@llvm/pr-subscribers-llvm-ir
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-debuginfo

Author: Jeremy Morse (jmorse)

Changes

With this, I get a clean test suite running under RemoveDIs, the non-intrinsic representation of debug-info, including under asan. We've previously established that we generate identical binaries for some large projects, so this i just edge-case cleanup. The changes:

• CodeGenPrepare fixups need to apply to dbg.assigns as well as dbg.values (a dbg.assign is a dbg.value).
• Pin a test for constant-deletion to intrinsic debug-info: this very rare scenario uses a different kill-location sigil in dbg.value mode to RemoveDIs mode, which generates spurious test differences.
• Suppress a memory leak in a unit test: the code for dealing with trailing debug-info in a block is necessarily fiddly, leading to this leak when testing it. Developer-facing interfaces for moving instructions around always deal with this behind the scenes.
• SROA, when replacing some vector-loads, needs to insert the replacement loads ahead of any debug-info records so that their values remain dominated by a definition. Set the head-bit indicating our insertion should come before debug-info.

---

Full diff: https://github.com/llvm/llvm-project/pull/81125.diff

6 Files Affected:

• (modified) llvm/lib/CodeGen/CodeGenPrepare.cpp (+2-1)
• (modified) llvm/lib/Transforms/Scalar/SROA.cpp (+6-1)
• (modified) llvm/test/DebugInfo/Generic/assignment-tracking/codegenprepare/sunk-addr.ll (+5)
• (modified) llvm/test/Transforms/GlobalOpt/localize-constexpr-debuginfo.ll (+6-1)
• (modified) llvm/test/Transforms/SROA/vector-promotion.ll (+4)
• (modified) llvm/unittests/IR/BasicBlockDbgInfoTest.cpp (+1)

diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 5383b15c1c7f5d..09c4922d8822cc 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -8455,7 +8455,8 @@ bool CodeGenPrepare::fixupDPValuesOnInst(Instruction &I) {
 // FIXME: should updating debug-info really cause the "changed" flag to fire,
 // which can cause a function to be reprocessed?
 bool CodeGenPrepare::fixupDPValue(DPValue &DPV) {
-  if (DPV.Type != DPValue::LocationType::Value)
+  if (DPV.Type != DPValue::LocationType::Value &&
+      DPV.Type != DPValue::LocationType::Assign)
     return false;
 
   // Does this DPValue refer to a sunk address calculation?
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index bdbaf4f55c96d0..e92e2459ead551 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -2956,7 +2956,12 @@ class AllocaSliceRewriter : public InstVisitor<AllocaSliceRewriter, bool> {
       assert(DL.typeSizeEqualsStoreSize(LI.getType()) &&
              "Non-byte-multiple bit width");
       // Move the insertion point just past the load so that we can refer to it.
-      IRB.SetInsertPoint(&*std::next(BasicBlock::iterator(&LI)));
+      BasicBlock::iterator LIIt = std::next(LI.getIterator());
+      // Ensure the insertion point comes before any debug-info immediately
+      // after the load, so that variable values referring to the load are
+      // dominated by it.
+      LIIt.setHeadBit(true);
+      IRB.SetInsertPoint(LI.getParent(), LIIt);
       // Create a placeholder value with the same type as LI to use as the
       // basis for the new value. This allows us to replace the uses of LI with
       // the computed value, and then replace the placeholder with LI, leaving
diff --git a/llvm/test/DebugInfo/Generic/assignment-tracking/codegenprepare/sunk-addr.ll b/llvm/test/DebugInfo/Generic/assignment-tracking/codegenprepare/sunk-addr.ll
index 70548465828079..8b226aa6633060 100644
--- a/llvm/test/DebugInfo/Generic/assignment-tracking/codegenprepare/sunk-addr.ll
+++ b/llvm/test/DebugInfo/Generic/assignment-tracking/codegenprepare/sunk-addr.ll
@@ -3,6 +3,11 @@
 ; RUN:   -mtriple=x86_64-unknown-unknown %s -o - \
 ; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg."
 
+;; Test with RemoveDIs non-intrinsic debug-info too.
+; RUN: llc -start-before=codegenprepare -stop-after=codegenprepare \
+; RUN:   -mtriple=x86_64-unknown-unknown %s -o - --try-experimental-debuginfo-iterators \
+; RUN: | FileCheck %s --implicit-check-not="call void @llvm.dbg."
+
 ;; Check that when CodeGenPrepare moves an address computation to a block it's
 ;; used in its dbg.assign uses are updated.
 ;;
diff --git a/llvm/test/Transforms/GlobalOpt/localize-constexpr-debuginfo.ll b/llvm/test/Transforms/GlobalOpt/localize-constexpr-debuginfo.ll
index 18dc038fce66af..5d6cc7db5a41f3 100644
--- a/llvm/test/Transforms/GlobalOpt/localize-constexpr-debuginfo.ll
+++ b/llvm/test/Transforms/GlobalOpt/localize-constexpr-debuginfo.ll
@@ -1,4 +1,9 @@
-; RUN: opt -S < %s -passes=globalopt | FileCheck %s
+; RUN: opt -S < %s -passes=globalopt --experimental-debuginfo-iterators=false | FileCheck %s
+;; FIXME: this test is pinned to not use RemoveDIs non-intrinsic debug-info.
+;; Constant-deletion takes a slightly different path and (correctly) replaces
+;; the operand of the debug-info record with poison instead of a null pointer.
+;; This is a spurious test difference that we'll suppress for turning RemoveDIs
+;; on.
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
diff --git a/llvm/test/Transforms/SROA/vector-promotion.ll b/llvm/test/Transforms/SROA/vector-promotion.ll
index e2aa1e2ee1c708..e48dd5bb392082 100644
--- a/llvm/test/Transforms/SROA/vector-promotion.ll
+++ b/llvm/test/Transforms/SROA/vector-promotion.ll
@@ -2,6 +2,10 @@
 ; RUN: opt < %s -passes='sroa<preserve-cfg>' -S | FileCheck %s --check-prefixes=CHECK,CHECK-PRESERVE-CFG
 ; RUN: opt < %s -passes='sroa<modify-cfg>' -S | FileCheck %s --check-prefixes=CHECK,CHECK-MODIFY-CFG
 ; RUN: opt < %s -passes=debugify,sroa -S | FileCheck %s --check-prefix=DEBUG
+;;  Ensure that these work with non-intrinsic variable locations.
+; RUN: opt < %s -passes='sroa<preserve-cfg>' -S --try-experimental-debuginfo-iterators | FileCheck %s --check-prefixes=CHECK,CHECK-PRESERVE-CFG
+; RUN: opt < %s -passes='sroa<modify-cfg>' -S --try-experimental-debuginfo-iterators | FileCheck %s --check-prefixes=CHECK,CHECK-MODIFY-CFG
+; RUN: opt < %s -passes=debugify,sroa -S --try-experimental-debuginfo-iterators | FileCheck %s --check-prefix=DEBUG
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-n8:16:32:64"
 
 %S1 = type { i64, [42 x float] }
diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
index 827b4a9c0cc323..ef2b288d859a7a 100644
--- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
+++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp
@@ -1476,6 +1476,7 @@ TEST(BasicBlockDbgInfoTest, DbgSpliceToEmpty2) {
   // ... except for some dangling DPValues.
   EXPECT_NE(Exit.getTrailingDPValues(), nullptr);
   EXPECT_FALSE(Exit.getTrailingDPValues()->empty());
+  Exit.getTrailingDPValues()->eraseFromParent();
   Exit.deleteTrailingDPValues();
 
   UseNewDbgInfoFormat = false;

[jryans]

Looks good to me, thanks! 😄

[jmorse]

Muchas gracias!