[RemoveDIs] Make verify-uselistorder preserve the input debug info format

[SLTozer]

Verify-uselistorder wants to take some input IR and verify that the uselist order is stable after roundtripping to bitcode and assembly. This is disrupted if the file is converted between the new and old debug info formats after parsing - while there's no functional difference, the change to the in-memory representation of the IR modifies the uselist. This patch changes verify-uselistorder to not convert input files between debug info formats by default, preventing changes from being made to the file being checked. In addition, this patch makes it so that when we do print IR in the new debug info format to bitcode or assembly, we delete any lingering debug intrinsic declarations, ensuring that we don't write uselist entries for them.

[llvmbot]

@llvm/pr-subscribers-debuginfo

@llvm/pr-subscribers-llvm-transforms

Author: Stephen Tozer (SLTozer)

Changes

Verify-uselistorder wants to take some input IR and verify that the uselist order is stable after roundtripping to bitcode and assembly. This is disrupted if the file is converted between the new and old debug info formats after parsing - while there's no functional difference, the change to the in-memory representation of the IR modifies the uselist. This patch changes verify-uselistorder to not convert input files between debug info formats by default, preventing changes from being made to the file being checked. In addition, this patch makes it so that when we do print IR in the new debug info format to bitcode or assembly, we delete any lingering debug intrinsic declarations, ensuring that we don't write uselist entries for them.

---

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

5 Files Affected:

• (modified) llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp (+4)
• (modified) llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp (+2)
• (modified) llvm/tools/llvm-as/llvm-as.cpp (+3-1)
• (modified) llvm/tools/llvm-dis/llvm-dis.cpp (+2)
• (modified) llvm/tools/verify-uselistorder/verify-uselistorder.cpp (+2-14)

diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
index 4f2486c963e987..048c72bce7c0f3 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
@@ -23,6 +23,8 @@ extern bool WriteNewDbgInfoFormatToBitcode;
 PreservedAnalyses BitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
   ScopedDbgInfoFormatSetter FormatSetter(M, M.IsNewDbgInfoFormat &&
                                                 WriteNewDbgInfoFormatToBitcode);
+  if (M.IsNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode)
+    M.removeDebugIntrinsicDeclarations();
 
   const ModuleSummaryIndex *Index =
       EmitSummaryIndex ? &(AM.getResult<ModuleSummaryIndexAnalysis>(M))
@@ -54,6 +56,8 @@ namespace {
     bool runOnModule(Module &M) override {
       ScopedDbgInfoFormatSetter FormatSetter(
           M, M.IsNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode);
+      if (M.IsNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode)
+        M.removeDebugIntrinsicDeclarations();
 
       WriteBitcodeToFile(M, OS, ShouldPreserveUseListOrder, /*Index=*/nullptr,
                          /*EmitModuleHash=*/false);
diff --git a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
index 4df18c8249277f..777c1984555335 100644
--- a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -585,6 +585,8 @@ llvm::ThinLTOBitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
   // debug-info, convert to dbg.values before writing out.
   ScopedDbgInfoFormatSetter FormatSetter(M, M.IsNewDbgInfoFormat &&
                                                 WriteNewDbgInfoFormatToBitcode);
+  if (M.IsNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode)
+    M.removeDebugIntrinsicDeclarations();
 
   bool Changed = writeThinLTOBitcode(
       OS, ThinLinkOS,
diff --git a/llvm/tools/llvm-as/llvm-as.cpp b/llvm/tools/llvm-as/llvm-as.cpp
index fd852563838f37..e48e3f4d22c123 100644
--- a/llvm/tools/llvm-as/llvm-as.cpp
+++ b/llvm/tools/llvm-as/llvm-as.cpp
@@ -143,8 +143,10 @@ int main(int argc, char **argv) {
 
   // Convert to new debug format if requested.
   assert(!M->IsNewDbgInfoFormat && "Unexpectedly in new debug mode");
-  if (UseNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode)
+  if (UseNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode) {
     M->convertToNewDbgValues();
+    M->removeDebugIntrinsicDeclarations();
+  }
 
   std::unique_ptr<ModuleSummaryIndex> Index = std::move(ModuleAndIndex.Index);
 
diff --git a/llvm/tools/llvm-dis/llvm-dis.cpp b/llvm/tools/llvm-dis/llvm-dis.cpp
index 6ad1c99568e4a5..fbbb5506e43e05 100644
--- a/llvm/tools/llvm-dis/llvm-dis.cpp
+++ b/llvm/tools/llvm-dis/llvm-dis.cpp
@@ -259,6 +259,8 @@ int main(int argc, char **argv) {
       if (!DontPrint) {
         if (M) {
           ScopedDbgInfoFormatSetter FormatSetter(*M, WriteNewDbgInfoFormat);
+          if (WriteNewDbgInfoFormat)
+            M->removeDebugIntrinsicDeclarations();
           M->print(Out->os(), Annotator.get(), PreserveAssemblyUseListOrder);
         }
         if (Index)
diff --git a/llvm/tools/verify-uselistorder/verify-uselistorder.cpp b/llvm/tools/verify-uselistorder/verify-uselistorder.cpp
index cb07dede1d1374..b239190e4dec09 100644
--- a/llvm/tools/verify-uselistorder/verify-uselistorder.cpp
+++ b/llvm/tools/verify-uselistorder/verify-uselistorder.cpp
@@ -68,7 +68,7 @@ static cl::opt<unsigned>
                 cl::desc("Number of times to shuffle and verify use-lists"),
                 cl::init(1), cl::cat(Cat));
 
-extern cl::opt<cl::boolOrDefault> LoadBitcodeIntoNewDbgInfoFormat;
+extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;
 
 namespace {
 
@@ -169,9 +169,6 @@ std::unique_ptr<Module> TempFile::readBitcode(LLVMContext &Context) const {
     return nullptr;
   }
 
-  // verify-uselistoder currently only supports old-style debug info mode.
-  // FIXME: Update mapping code for RemoveDIs.
-  ModuleOr.get()->setIsNewDbgInfoFormat(false);
   return std::move(ModuleOr.get());
 }
 
@@ -181,9 +178,6 @@ std::unique_ptr<Module> TempFile::readAssembly(LLVMContext &Context) const {
   std::unique_ptr<Module> M = parseAssemblyFile(Filename, Err, Context);
   if (!M.get())
     Err.print("verify-uselistorder", errs());
-  // verify-uselistoder currently only supports old-style debug info mode.
-  // FIXME: Update mapping code for RemoveDIs.
-  M->setIsNewDbgInfoFormat(false);
   return M;
 }
 
@@ -536,6 +530,7 @@ static void reverseUseLists(Module &M) {
 }
 
 int main(int argc, char **argv) {
+  PreserveInputDbgFormat = cl::boolOrDefault::BOU_TRUE;
   InitLLVM X(argc, argv);
 
   // Enable debug stream buffering.
@@ -545,18 +540,11 @@ int main(int argc, char **argv) {
   cl::ParseCommandLineOptions(argc, argv,
                               "llvm tool to verify use-list order\n");
 
-  // Do not load bitcode into the new debug info format by default.
-  if (LoadBitcodeIntoNewDbgInfoFormat == cl::boolOrDefault::BOU_UNSET)
-    LoadBitcodeIntoNewDbgInfoFormat = cl::boolOrDefault::BOU_FALSE;
-
   LLVMContext Context;
   SMDiagnostic Err;
 
   // Load the input module...
   std::unique_ptr<Module> M = parseIRFile(InputFilename, Err, Context);
-  // verify-uselistoder currently only supports old-style debug info mode.
-  // FIXME: Update mapping code for RemoveDIs.
-  M->setIsNewDbgInfoFormat(false);
 
   if (!M.get()) {
     Err.print(argv[0], errs());

[OCHyams]

Should the conversion function unconditionally call removeDebugIntrinsicDeclarations instead, or does that cause problems with existing tests?

[SLTozer]

It causes problems with existing tests, yes - when all the test IR gets updated (as we move to writing new-format by default) it'll be moved into the conversion function, but otherwise it causes intrinsic declarations to be reordered, which is awkward for tests and feels like a wrong behaviour.

[OCHyams]

These used to be asserts (checking we're not using the new mode). IMO we should consider adding those back in again until verify-uselistorder actually supports the new format, as AFAIUI right now the DbgRecords are just ignored (not mapped, i.e., their ordering isn't checked by verify-uselistorder). WDYT?

[OCHyams]

Does the commit "Enumerate values used by debug records in verify-uselistorder" mean that it now supports DbgRecords? If so, my previous comment:

These used to be asserts (checking we're not using the new mode). IMO we should consider adding those back in again until verify-uselistorder actually supports the new format, as AFAIUI right now the DbgRecords are just ignored (not mapped, i.e., their ordering isn't checked by verify-uselistorder). WDYT?

is nullified and can be ignored.

[SLTozer]

Does the commit

Yeah, that's the intention: all I think is required functionally is to iterate over the values used in debug records; the only debug-record content that could influence verify-uselistorder is if they contain constant values that aren't used anywhere else, but that can happen, so the updated version accounts for that.

[OCHyams]

LGTM!