[llvm] [RemoveDIs] Don't convert debug-info in bitcode-loading just now (PR #80865)

[Jeremy Morse via llvm-commits]

https://github.com/jmorse updated https://github.com/llvm/llvm-project/pull/80865

>From eec3c258faeaf3aa4ae1a7ce91d88f1fa30638e8 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Tue, 6 Feb 2024 15:10:15 +0000
Subject: [PATCH 1/2] [RemoveDIs] Don't convert debug-info in bitcode-loading
 just now

We've been building and testing this no-debug-intrinsic work inside of the
pass manager for a while, so that optimisation passes get exercised and
tested when we turn it on. However, by converting to the non-intrinsic form
in the bitcode loader, we accidentally caused all parts of LLVM to
potentially see non-intrinsic debug-info.

Seeing how we're trying to turn things on incrementally, it was a mistake
to go this far this fast: we can instead just focus on enabling during
optimisations for the moment, then all the other parts of LLVM later.
---
 llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 5b233fb365fe27..db86df2fdc72ea 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -6632,9 +6632,6 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
   if (Error Err = materializeMetadata())
     return Err;
 
-  bool NewDebugInfoRequested = F->IsNewDbgInfoFormat;
-  F->IsNewDbgInfoFormat = false;
-
   // Move the bit stream to the saved position of the deferred function body.
   if (Error JumpFailed = Stream.JumpToBit(DFII->second))
     return JumpFailed;
@@ -6710,14 +6707,6 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
   // Look for functions that rely on old function attribute behavior.
   UpgradeFunctionAttributes(*F);
 
-  // If we've materialized a function set up in "new" debug-info mode, the
-  // contents just loaded will still be in dbg.value mode. Switch to the new
-  // mode now. NB: we can add more complicated logic here in the future to
-  // correctly identify when we do and don't need to autoupgrade.
-  if (NewDebugInfoRequested) {
-    F->convertToNewDbgValues();
-  }
-
   // Bring in any functions that this function forward-referenced via
   // blockaddresses.
   return materializeForwardReferencedFunctions();

>From 89e1800c809f574e604169ef0e67f333646e5154 Mon Sep 17 00:00:00 2001
From: Jeremy Morse <jeremy.morse at sony.com>
Date: Tue, 6 Feb 2024 18:07:01 +0000
Subject: [PATCH 2/2] Strip out more undesired bitcode conversion

This extra hunk in BitcodeReader.cpp is also expanding how much of LLVM can
see RemoveDIs, and so is undesireable. At the same time, add a conversion
to the IRMover: when running thinlto links, the function importer pass can
run in a RemoveDIs context and tries to merge in functions from
non-RemoveDIs contexts. If that's the case, do the conversion.

This is on-theme because it's only going to trigger when the destination
module is in RemoveDIs mode, which will only be inside the pass manager.
---
 llvm/lib/Bitcode/Reader/BitcodeReader.cpp | 8 --------
 llvm/lib/Linker/IRMover.cpp               | 2 ++
 llvm/test/ThinLTO/X86/crash_debuginfo.ll  | 3 +++
 3 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index db86df2fdc72ea..515a1d0caa0415 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -8031,14 +8031,6 @@ BitcodeModule::getModuleImpl(LLVMContext &Context, bool MaterializeAll,
       return std::move(Err);
   }
 
-  // If we are operating in a "new debug-info" context, upgrade the debug-info
-  // in the loaded module. This is a transitional approach as we enable "new"
-  // debug-info in LLVM, which will eventually be pushed down into the
-  // autoupgrade path once the bitcode-encoding is finalised. Non-materialised
-  // functions will be upgraded in the materialize method.
-  if (UseNewDbgInfoFormat && !M->IsNewDbgInfoFormat)
-    M->convertToNewDbgValues();
-
   return std::move(M);
 }
 
diff --git a/llvm/lib/Linker/IRMover.cpp b/llvm/lib/Linker/IRMover.cpp
index 8cc0f7fb909916..37d21119447b9c 100644
--- a/llvm/lib/Linker/IRMover.cpp
+++ b/llvm/lib/Linker/IRMover.cpp
@@ -1775,6 +1775,8 @@ IRMover::IRMover(Module &M) : Composite(M) {
 Error IRMover::move(std::unique_ptr<Module> Src,
                     ArrayRef<GlobalValue *> ValuesToLink,
                     LazyCallback AddLazyFor, bool IsPerformingImport) {
+  if (getModule().IsNewDbgInfoFormat)
+    Src->convertToNewDbgValues();
   IRLinker TheIRLinker(Composite, SharedMDs, IdentifiedStructTypes,
                        std::move(Src), ValuesToLink, std::move(AddLazyFor),
                        IsPerformingImport);
diff --git a/llvm/test/ThinLTO/X86/crash_debuginfo.ll b/llvm/test/ThinLTO/X86/crash_debuginfo.ll
index 9e6f9aa2292f2b..1377942e566dfc 100644
--- a/llvm/test/ThinLTO/X86/crash_debuginfo.ll
+++ b/llvm/test/ThinLTO/X86/crash_debuginfo.ll
@@ -4,6 +4,9 @@
 ; RUN: opt -passes=function-import,inline -summary-file %t-index.thinlto.bc %t-dst.bc -o %t.out
 ; RUN: llvm-nm -U %t.out | FileCheck %s --implicit-check-not=_bar
 
+; Ensure we can do the same thing in RemoveDIs mode.
+; RUN: opt -passes=function-import,inline -summary-file %t-index.thinlto.bc %t-dst.bc -o %t.out --try-experimental-debuginfo-iterators
+
 ; Verify that we import bar and inline it. It use to crash importing due to ODR type uniquing
 ; CHECK: _foo