[DebugInfo] Remove old decls when converting DI#194964
Conversation
We were trying to remove declarations of old debug intrinsics whenever printing modules or writing them to file. This is no longer necessary as we use the new-style debug values exclusively now, other than when a target pass specifically converts back to the old style. If a target pass does that, removing the intrinsics is not right as the intrinsics' users will still linger. This change should be NFC except for the experimental DirectX target where we do exactly that. Fixes llvm#194884
|
@llvm/pr-subscribers-backend-directx @llvm/pr-subscribers-mlir-llvm Author: Harald van Dijk (hvdijk) ChangesWe were trying to remove declarations of old debug intrinsics whenever printing modules or writing them to file. This is no longer necessary as we use the new-style debug values exclusively now, other than when a target pass specifically converts back to the old style. If a target pass does that, removing the intrinsics is not right as the intrinsics' users will still linger. This change should be NFC except for the experimental DirectX target where we do exactly that. Fixes #194884 Full diff: https://github.com/llvm/llvm-project/pull/194964.diff 11 Files Affected:
diff --git a/llvm/include/llvm/IR/Module.h b/llvm/include/llvm/IR/Module.h
index 7156a83c9f3cc..1218d47dc0e46 100644
--- a/llvm/include/llvm/IR/Module.h
+++ b/llvm/include/llvm/IR/Module.h
@@ -225,6 +225,8 @@ class LLVM_ABI Module {
for (auto &F : *this) {
F.convertToNewDbgValues();
}
+
+ removeDebugIntrinsicDeclarations();
}
/// \see BasicBlock::convertFromNewDbgValues.
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
index e48f735ded831..0e823ef58ea48 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
@@ -19,8 +19,6 @@
using namespace llvm;
PreservedAnalyses BitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
- M.removeDebugIntrinsicDeclarations();
-
const ModuleSummaryIndex *Index =
EmitSummaryIndex ? &(AM.getResult<ModuleSummaryIndexAnalysis>(M))
: nullptr;
@@ -49,8 +47,6 @@ namespace {
StringRef getPassName() const override { return "Bitcode Writer"; }
bool runOnModule(Module &M) override {
- M.removeDebugIntrinsicDeclarations();
-
WriteBitcodeToFile(M, OS, ShouldPreserveUseListOrder, /*Index=*/nullptr,
/*EmitModuleHash=*/false);
diff --git a/llvm/lib/IR/IRPrintingPasses.cpp b/llvm/lib/IR/IRPrintingPasses.cpp
index 5c062800198fc..43671b0b9a1a3 100644
--- a/llvm/lib/IR/IRPrintingPasses.cpp
+++ b/llvm/lib/IR/IRPrintingPasses.cpp
@@ -40,10 +40,6 @@ class PrintModulePassWrapper : public ModulePass {
ShouldPreserveUseListOrder(ShouldPreserveUseListOrder) {}
bool runOnModule(Module &M) override {
- // Remove intrinsic declarations when printing in the new format.
- // TODO: consider removing this as debug-intrinsics are gone.
- M.removeDebugIntrinsicDeclarations();
-
if (llvm::isFunctionInPrintList("*")) {
if (!Banner.empty())
OS << Banner << "\n";
diff --git a/llvm/lib/IRPrinter/IRPrintingPasses.cpp b/llvm/lib/IRPrinter/IRPrintingPasses.cpp
index 81ad284ea1642..adb192ac4a916 100644
--- a/llvm/lib/IRPrinter/IRPrintingPasses.cpp
+++ b/llvm/lib/IRPrinter/IRPrintingPasses.cpp
@@ -32,10 +32,6 @@ PrintModulePass::PrintModulePass(raw_ostream &OS, const std::string &Banner,
EmitSummaryIndex(EmitSummaryIndex) {}
PreservedAnalyses PrintModulePass::run(Module &M, ModuleAnalysisManager &AM) {
- // Remove intrinsic declarations when printing in the new format.
- // TODO: consider removing this now that debug intrinsics are gone.
- M.removeDebugIntrinsicDeclarations();
-
if (llvm::isFunctionInPrintList("*")) {
if (!Banner.empty())
OS << Banner << "\n";
diff --git a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
index fb9969620f7f8..3a6851eb60272 100644
--- a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -599,8 +599,6 @@ llvm::ThinLTOBitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
FunctionAnalysisManager &FAM =
AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
- M.removeDebugIntrinsicDeclarations();
-
bool Changed = writeThinLTOBitcode(
OS, ThinLinkOS,
[&FAM](Function &F) -> AAResults & {
diff --git a/llvm/test/CodeGen/DirectX/debug-info.ll b/llvm/test/CodeGen/DirectX/debug-info.ll
new file mode 100644
index 0000000000000..1f30127e09ebc
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/debug-info.ll
@@ -0,0 +1,62 @@
+; RUN: llc --filetype=asm %s -o - | FileCheck %s
+
+target triple = "dxil-unknown-shadermodel6.7-library"
+target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
+
+; CHECK: define dso_local float @fma(float %0, float %1, float %2) local_unnamed_addr #0 !dbg [[Fn:[!][0-9]+]]
+define dso_local float @fma(float %0, float %1, float %2) local_unnamed_addr #0 !dbg !6 {
+; CHECK-NEXT: call void @llvm.dbg.value(metadata float %0, metadata [[VarX:[!][0-9]+]], metadata !DIExpression()), !dbg [[Line1:[!][0-9]+]]
+; CHECK-NEXT: call void @llvm.dbg.value(metadata float %1, metadata [[VarY:[!][0-9]+]], metadata !DIExpression()), !dbg [[Line1]]
+; CHECK-NEXT: call void @llvm.dbg.value(metadata float %2, metadata [[VarZ:[!][0-9]+]], metadata !DIExpression()), !dbg [[Line1]]
+ call void @llvm.dbg.value(metadata float %0, metadata !11, metadata !DIExpression()), !dbg !14
+ call void @llvm.dbg.value(metadata float %1, metadata !12, metadata !DIExpression()), !dbg !14
+ call void @llvm.dbg.value(metadata float %2, metadata !13, metadata !DIExpression()), !dbg !14
+; CHECK-NEXT: %4 = fmul float %0, %1, !dbg [[Line2:[!][0-9]+]]
+; CHECK-NEXT: %5 = fadd float %4, %2, !dbg [[Line3:[!][0-9]+]]
+ %4 = fmul float %0, %1, !dbg !15
+ %5 = fadd float %4, %2, !dbg !16
+ ret float %5, !dbg !17
+}
+
+
+attributes #0 = { norecurse nounwind readnone willreturn "hlsl.export" }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.ident = !{!5}
+
+; Other tests verify that we come back with reasonable structure for the debug
+; info types, this test just needs to ensure they are there.
+; The patch this is paired with fixes a bug where function debug info wasn't
+; being emitted correctly even though other tests verified the MD would be
+; emitted if it was referenced as module metadata.
+
+; CHECK: !0 = distinct !DICompileUnit
+; CHECK-NEXT: !1 = !DIFile(filename:
+; CHECK: [[Fn]] = distinct !DISubprogram(name: "fma",
+; CHECK: [[VarX]] = !DILocalVariable(
+; CHECK: [[VarY]] = !DILocalVariable(
+; CHECK: [[VarZ]] = !DILocalVariable(
+; CHECK-NEXT: [[Line1]] = !DILocation(line:
+; CHECK-NEXT: [[Line2]] = !DILocation(line:
+; CHECK-NEXT: [[Line3]] = !DILocation(line:
+; CHECK-NEXT: {{[!][0-9]+}} = !DILocation(line:
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "in.c", directory: "dir")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 2}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{!"Some Compiler"}
+!6 = distinct !DISubprogram(name: "fma", scope: !1, file: !1, line: 1, type: !7, scopeLine: 1, flags: DIFlagPrototyped, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !10)
+!7 = !DISubroutineType(types: !8)
+!8 = !{!9, !9, !9, !9}
+!9 = !DIBasicType(name: "float", size: 32, encoding: DW_ATE_float)
+!10 = !{!11, !12, !13}
+!11 = !DILocalVariable(name: "x", arg: 1, scope: !6, file: !1, line: 1, type: !9)
+!12 = !DILocalVariable(name: "y", arg: 2, scope: !6, file: !1, line: 1, type: !9)
+!13 = !DILocalVariable(name: "z", arg: 3, scope: !6, file: !1, line: 1, type: !9)
+!14 = !DILocation(line: 0, scope: !6)
+!15 = !DILocation(line: 2, column: 12, scope: !6)
+!16 = !DILocation(line: 2, column: 16, scope: !6)
+!17 = !DILocation(line: 2, column: 3, scope: !6)
diff --git a/llvm/tools/llvm-as/llvm-as.cpp b/llvm/tools/llvm-as/llvm-as.cpp
index 200e6a5c564da..5713f28397834 100644
--- a/llvm/tools/llvm-as/llvm-as.cpp
+++ b/llvm/tools/llvm-as/llvm-as.cpp
@@ -134,8 +134,6 @@ int main(int argc, char **argv) {
return 1;
}
- M->removeDebugIntrinsicDeclarations();
-
std::unique_ptr<ModuleSummaryIndex> Index = std::move(ModuleAndIndex.Index);
if (!DisableVerify) {
diff --git a/llvm/tools/llvm-dis/llvm-dis.cpp b/llvm/tools/llvm-dis/llvm-dis.cpp
index 818185ff5c6ca..a961f9cb0f7dd 100644
--- a/llvm/tools/llvm-dis/llvm-dis.cpp
+++ b/llvm/tools/llvm-dis/llvm-dis.cpp
@@ -263,7 +263,6 @@ int main(int argc, char **argv) {
// All that llvm-dis does is write the assembly to a file.
if (!DontPrint) {
if (M) {
- M->removeDebugIntrinsicDeclarations();
M->print(Out->os(), Annotator.get(),
/* ShouldPreserveUseListOrder */ false);
}
diff --git a/llvm/tools/llvm-link/llvm-link.cpp b/llvm/tools/llvm-link/llvm-link.cpp
index b264f9e0d3e93..f9fac9032039c 100644
--- a/llvm/tools/llvm-link/llvm-link.cpp
+++ b/llvm/tools/llvm-link/llvm-link.cpp
@@ -516,7 +516,6 @@ int main(int argc, char **argv) {
if (Verbose)
errs() << "Writing bitcode...\n";
- Composite->removeDebugIntrinsicDeclarations();
if (OutputAssembly) {
Composite->print(Out.os(), nullptr, /* ShouldPreserveUseListOrder */ false);
} else if (Force || !CheckBitcodeOutputToConsole(Out.os())) {
diff --git a/mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp b/mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp
index 8e6f5c7191daa..fef191a302ac8 100644
--- a/mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp
+++ b/mlir/lib/Target/LLVMIR/ConvertToLLVMIR.cpp
@@ -31,7 +31,6 @@ void registerToLLVMIRTranslation() {
if (!llvmModule)
return failure();
- llvmModule->removeDebugIntrinsicDeclarations();
llvmModule->print(output, nullptr);
return success();
},
diff --git a/mlir/test/lib/Dialect/Test/TestToLLVMIRTranslation.cpp b/mlir/test/lib/Dialect/Test/TestToLLVMIRTranslation.cpp
index 7831b27e94d71..19624c71f841e 100644
--- a/mlir/test/lib/Dialect/Test/TestToLLVMIRTranslation.cpp
+++ b/mlir/test/lib/Dialect/Test/TestToLLVMIRTranslation.cpp
@@ -123,7 +123,6 @@ void registerTestToLLVMIR() {
if (!llvmModule)
return failure();
- llvmModule->removeDebugIntrinsicDeclarations();
llvmModule->print(output, nullptr);
return success();
},
|
|
@jmorse Am I right that this should be NFC for every target other than DirectX, or do you know if I am missing something? |
jmorse
left a comment
There was a problem hiding this comment.
LGTM, and thanks for flushing this fault out. My inline comments are purely about signalling the intended coverage to future readers of the test.
|
Thanks for the feedback and approval, I'll wait a little bit in case there's additional feedback on the updated comments, otherwise I'll merge tomorrow. |
|
The failing CI is unrelated and is due to bad interaction with #194858, I see @joaosaffran is already looking into that in #196430. |
We were trying to remove declarations of old debug intrinsics whenever printing modules or writing them to file. This is no longer necessary as we use the new-style debug values exclusively now, other than when a target pass specifically converts back to the old style. If a target pass does that, removing the intrinsics is not right as the intrinsics' users will still linger. This change should be NFC except for the experimental DirectX target where we do exactly that. Fixes llvm#194884
We were trying to remove declarations of old debug intrinsics whenever printing modules or writing them to file. This is no longer necessary as we use the new-style debug values exclusively now, other than when a target pass specifically converts back to the old style. If a target pass does that, removing the intrinsics is not right as the intrinsics' users will still linger. This change should be NFC except for the experimental DirectX target where we do exactly that. Fixes llvm#194884
We were trying to remove declarations of old debug intrinsics whenever printing modules or writing them to file. This is no longer necessary as we use the new-style debug values exclusively now, other than when a target pass specifically converts back to the old style. If a target pass does that, removing the intrinsics is not right as the intrinsics' users will still linger. This change should be NFC except for the experimental DirectX target where we do exactly that. Fixes llvm#194884
We were trying to remove declarations of old debug intrinsics whenever printing modules or writing them to file. This is no longer necessary as we use the new-style debug values exclusively now, other than when a target pass specifically converts back to the old style. If a target pass does that, removing the intrinsics is not right as the intrinsics' users will still linger.
This change should be NFC except for the experimental DirectX target where we do exactly that.
Fixes #194884