[DebugInfo][RemoveDIs] Extend intrinsic-conversion in debugify #80861
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
A while back the entry/exit points of debugify were instrumented with conversion functions to/from non-intrinsic-form debug-info. This is the path of least resistance to incrementally converting parts of LLVM to use the new format. However, it turns out that debugify registers callbacks with the pass manager and can be fed non-intrinsic form debug-info. Thus: this patch wraps each of the four major debugify functions with the convertion utilities, and extends test coverage to a test that exposes this problem. (An alternative would be to put this code in the callback lambdas, but then it would be fighting pass manager abstractions of what type the IR has). Handily debugify has been designed to record the /meaning/ of debug-info rather than take pointers to intrinsics and the like, so the storage mechanism for debug-info is transparent to it!
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesA while back the entry/exit points of debugify were instrumented with conversion functions to/from non-intrinsic-form debug-info. This is the path of least resistance to incrementally converting parts of LLVM to use the new format. However, it turns out that debugify registers callbacks with the pass manager and can be fed non-intrinsic form debug-info. Thus: this patch wraps each of the four major debugify functions with the convertion utilities, and extends test coverage to a test that exposes this problem. (An alternative would be to put this code in the callback lambdas, but then it would be fighting pass manager abstractions of what type the IR has). Handily debugify has been designed to record the /meaning/ of debug-info rather than take pointers to intrinsics and the like, so the storage mechanism for debug-info is transparent to it! Full diff: https://github.com/llvm/llvm-project/pull/80861.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/Debugify.cpp b/llvm/lib/Transforms/Utils/Debugify.cpp
index d0cc603426d2a7..be45c5769b2ae3 100644
--- a/llvm/lib/Transforms/Utils/Debugify.cpp
+++ b/llvm/lib/Transforms/Utils/Debugify.cpp
@@ -87,6 +87,10 @@ bool llvm::applyDebugifyMetadata(
return false;
}
+ bool NewDebugMode = M.IsNewDbgInfoFormat;
+ if (NewDebugMode)
+ M.convertFromNewDbgValues();
+
DIBuilder DIB(M);
LLVMContext &Ctx = M.getContext();
auto *Int32Ty = Type::getInt32Ty(Ctx);
@@ -210,6 +214,9 @@ bool llvm::applyDebugifyMetadata(
if (!M.getModuleFlag(DIVersionKey))
M.addModuleFlag(Module::Warning, DIVersionKey, DEBUG_METADATA_VERSION);
+ if (NewDebugMode)
+ M.convertToNewDbgValues();
+
return true;
}
@@ -304,6 +311,10 @@ bool llvm::collectDebugInfoMetadata(Module &M,
return false;
}
+ bool NewDebugMode = M.IsNewDbgInfoFormat;
+ if (NewDebugMode)
+ M.convertFromNewDbgValues();
+
uint64_t FunctionsCnt = DebugInfoBeforePass.DIFunctions.size();
// Visit each instruction.
for (Function &F : Functions) {
@@ -368,6 +379,9 @@ bool llvm::collectDebugInfoMetadata(Module &M,
}
}
+ if (NewDebugMode)
+ M.convertToNewDbgValues();
+
return true;
}
@@ -547,6 +561,10 @@ bool llvm::checkDebugInfoMetadata(Module &M,
return false;
}
+ bool NewDebugMode = M.IsNewDbgInfoFormat;
+ if (NewDebugMode)
+ M.convertFromNewDbgValues();
+
// Map the debug info holding DIs after a pass.
DebugInfoPerPass DebugInfoAfterPass;
@@ -657,6 +675,9 @@ bool llvm::checkDebugInfoMetadata(Module &M,
// the debugging information from the previous pass.
DebugInfoBeforePass = DebugInfoAfterPass;
+ if (NewDebugMode)
+ M.convertToNewDbgValues();
+
LLVM_DEBUG(dbgs() << "\n\n");
return Result;
}
@@ -714,6 +735,10 @@ bool checkDebugifyMetadata(Module &M,
return false;
}
+ bool NewDebugMode = M.IsNewDbgInfoFormat;
+ if (NewDebugMode)
+ M.convertFromNewDbgValues();
+
auto getDebugifyOperand = [&](unsigned Idx) -> unsigned {
return mdconst::extract<ConstantInt>(NMD->getOperand(Idx)->getOperand(0))
->getZExtValue();
@@ -791,24 +816,21 @@ bool checkDebugifyMetadata(Module &M,
dbg() << ": " << (HasErrors ? "FAIL" : "PASS") << '\n';
// Strip debugify metadata if required.
+ bool Ret = false;
if (Strip)
- return stripDebugifyMetadata(M);
+ Ret = stripDebugifyMetadata(M);
+
+ if (NewDebugMode)
+ M.convertToNewDbgValues();
- return false;
+ return Ret;
}
/// ModulePass for attaching synthetic debug info to everything, used with the
/// legacy module pass manager.
struct DebugifyModulePass : public ModulePass {
bool runOnModule(Module &M) override {
- bool NewDebugMode = M.IsNewDbgInfoFormat;
- if (NewDebugMode)
- M.convertFromNewDbgValues();
-
bool Result = applyDebugify(M, Mode, DebugInfoBeforePass, NameOfWrappedPass);
-
- if (NewDebugMode)
- M.convertToNewDbgValues();
return Result;
}
@@ -834,14 +856,7 @@ struct DebugifyModulePass : public ModulePass {
/// single function, used with the legacy module pass manager.
struct DebugifyFunctionPass : public FunctionPass {
bool runOnFunction(Function &F) override {
- bool NewDebugMode = F.IsNewDbgInfoFormat;
- if (NewDebugMode)
- F.convertFromNewDbgValues();
-
bool Result = applyDebugify(F, Mode, DebugInfoBeforePass, NameOfWrappedPass);
-
- if (NewDebugMode)
- F.convertToNewDbgValues();
return Result;
}
@@ -868,10 +883,6 @@ struct DebugifyFunctionPass : public FunctionPass {
/// legacy module pass manager.
struct CheckDebugifyModulePass : public ModulePass {
bool runOnModule(Module &M) override {
- bool NewDebugMode = M.IsNewDbgInfoFormat;
- if (NewDebugMode)
- M.convertFromNewDbgValues();
-
bool Result;
if (Mode == DebugifyMode::SyntheticDebugInfo)
Result = checkDebugifyMetadata(M, M.functions(), NameOfWrappedPass,
@@ -882,9 +893,6 @@ struct CheckDebugifyModulePass : public ModulePass {
"CheckModuleDebugify (original debuginfo)", NameOfWrappedPass,
OrigDIVerifyBugsReportFilePath);
- if (NewDebugMode)
- M.convertToNewDbgValues();
-
return Result;
}
@@ -918,10 +926,6 @@ struct CheckDebugifyModulePass : public ModulePass {
/// with the legacy module pass manager.
struct CheckDebugifyFunctionPass : public FunctionPass {
bool runOnFunction(Function &F) override {
- bool NewDebugMode = F.IsNewDbgInfoFormat;
- if (NewDebugMode)
- F.convertFromNewDbgValues();
-
Module &M = *F.getParent();
auto FuncIt = F.getIterator();
bool Result;
@@ -935,8 +939,6 @@ struct CheckDebugifyFunctionPass : public FunctionPass {
"CheckFunctionDebugify (original debuginfo)", NameOfWrappedPass,
OrigDIVerifyBugsReportFilePath);
- if (NewDebugMode)
- F.convertToNewDbgValues();
return Result;
}
@@ -1009,10 +1011,6 @@ createDebugifyFunctionPass(enum DebugifyMode Mode,
}
PreservedAnalyses NewPMDebugifyPass::run(Module &M, ModuleAnalysisManager &) {
- bool NewDebugMode = M.IsNewDbgInfoFormat;
- if (NewDebugMode)
- M.convertFromNewDbgValues();
-
if (Mode == DebugifyMode::SyntheticDebugInfo)
applyDebugifyMetadata(M, M.functions(),
"ModuleDebugify: ", /*ApplyToMF*/ nullptr);
@@ -1021,9 +1019,6 @@ PreservedAnalyses NewPMDebugifyPass::run(Module &M, ModuleAnalysisManager &) {
"ModuleDebugify (original debuginfo)",
NameOfWrappedPass);
- if (NewDebugMode)
- M.convertToNewDbgValues();
-
PreservedAnalyses PA;
PA.preserveSet<CFGAnalyses>();
return PA;
diff --git a/llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll b/llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll
index 65f24831bc89cb..b1ba4e15d49f87 100644
--- a/llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll
+++ b/llvm/test/Transforms/Util/Debugify/loc-only-original-mode.ll
@@ -14,6 +14,11 @@
; RUN: -verify-each-debuginfo-preserve \
; RUN: -debugify-func-limit=2 -S 2>&1 | FileCheck %s --check-prefix=CHECK-DROP
+;; Add some runlines that use RemoveDIs non-intrinsic debug-info, to check that
+;; variable preservation checking works.
+; RUN: opt < %s -passes=deadargelim --try-experimental-debuginfo-iterators \
+; RUN: -verify-each-debuginfo-preserve \
+; RUN: -debugify-level=location+variables -S 2>&1 | FileCheck %s --check-prefix=CHECK-DROP
; CHECK-NOT: drops dbg.value()/dbg.declare()
; CHECK-DROP: drops dbg.value()/dbg.declare()
@@ -22,10 +27,10 @@ target triple = "x86_64-unknown-linux-gnu"
define dso_local i32 @fn2(i32 %l, i32 %k) !dbg !7 {
entry:
- call void @llvm.dbg.value(metadata i32 %l, metadata !12, metadata !DIExpression()), !dbg !15
- call void @llvm.dbg.value(metadata i32 %k, metadata !13, metadata !DIExpression()), !dbg !15
+ tail call void @llvm.dbg.value(metadata i32 %l, metadata !12, metadata !DIExpression()), !dbg !15
+ tail call void @llvm.dbg.value(metadata i32 %k, metadata !13, metadata !DIExpression()), !dbg !15
%call = call i32 (...) @fn3(), !dbg !16
- call void @llvm.dbg.value(metadata i32 %call, metadata !14, metadata !DIExpression()), !dbg !15
+ tail call void @llvm.dbg.value(metadata i32 %call, metadata !14, metadata !DIExpression()), !dbg !15
ret i32 %call, !dbg !17
}
@@ -33,10 +38,10 @@ declare !dbg !18 dso_local i32 @fn3(...)
define dso_local i32 @fn(i32 %x, i32 %y) !dbg !22 {
entry:
- call void @llvm.dbg.value(metadata i32 %x, metadata !24, metadata !DIExpression()), !dbg !27
- call void @llvm.dbg.value(metadata i32 %y, metadata !25, metadata !DIExpression()), !dbg !27
+ tail call void @llvm.dbg.value(metadata i32 %x, metadata !24, metadata !DIExpression()), !dbg !27
+ tail call void @llvm.dbg.value(metadata i32 %y, metadata !25, metadata !DIExpression()), !dbg !27
%call = call i32 @fn2(i32 %x, i32 %y), !dbg !27
- call void @llvm.dbg.value(metadata i32 %call, metadata !26, metadata !DIExpression()), !dbg !27
+ tail call void @llvm.dbg.value(metadata i32 %call, metadata !26, metadata !DIExpression()), !dbg !27
%add = add nsw i32 %call, %x, !dbg !27
%add1 = add nsw i32 %add, %y, !dbg !27
ret i32 %add1, !dbg !27
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
jryans
approved these changes
Feb 6, 2024
|
Thanks for the review!,
100% yes -- but right now we're hitting difficulties keeping up with the rate-of-change in LLVM. It's unfortunately infeasible to have everything change in a single big-bang commit, so we've been trying to stage things in manageable chunks (and getting it wrong, see #80865). It's sort of feasible to enable this work for all optimisations soon (i.e.: actually tomorrow), and if that sticks, then we'll be able to circle back to areas we've walled off with these conversion calls. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A while back the entry/exit points of debugify were instrumented with conversion functions to/from non-intrinsic-form debug-info. This is the path of least resistance to incrementally converting parts of LLVM to use the new format. However, it turns out that debugify registers callbacks with the pass manager and can be fed non-intrinsic form debug-info. Thus: this patch wraps each of the four major debugify functions with the convertion utilities, and extends test coverage to a test that exposes this problem.
(An alternative would be to put this code in the callback lambdas, but then it would be fighting pass manager abstractions of what type the IR has).
Handily debugify has been designed to record the /meaning/ of debug-info rather than take pointers to intrinsics and the like, so the storage mechanism for debug-info is transparent to it!