[RemoveDIs] Don't convert debug-info in bitcode-loading just now #80865
Conversation
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/pr-subscribers-lto @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesWe'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. (@OCHyams @SLTozer this is what we've been talking about internally about "drawing the wrong line" through LLVM when staging these changes. Removing these hunks avoids every single tool in the repo having to immediately cope with RemoveDIs debug-info, and we can continue focusing on optimisations only). Full diff: https://github.com/llvm/llvm-project/pull/80865.diff 1 Files Affected:
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 5b233fb365fe2..db86df2fdc72e 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();
|
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.
| if (getModule().IsNewDbgInfoFormat) | ||
| Src->convertToNewDbgValues(); |
There was a problem hiding this comment.
Not familiar with the context of this function, but presumably Src will never be in the new format already because it's just been read from bitcode?
There was a problem hiding this comment.
"Probably yes" -- with this patch it's only the pass manager that's going to create modules in new-debug-info mode, so any other module created Should(TM) be in dbg.value format. This seems to be the case in the llvm-lit test suite.
I'm 99.5% confident of this; but we've also got a bunch of assertions for the IsNewDbgInfoFormat flag to detect codepaths where this isn't the case. If it turns out to be wrong we'll hopefully break-fast-break-hard.
|
(Merging on the basis of the above LGTM) |
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.
(@OCHyams @SLTozer this is what we've been talking about internally about "drawing the wrong line" through LLVM when staging these changes. Removing these hunks avoids every single tool in the repo having to immediately cope with RemoveDIs debug-info, and we can continue focusing on optimisations only).