[DebugInfo][RemoveDIs] Remove debug-intrinsic printing cmdline options #131855
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
During the transition from debug intrinsics to debug records, we used several different command line options to customise handling: the printing of debug records to bitcode and textual could be independent of how the debug-info was represented inside a module, whether the autoupgrader ran could be customised. This was all valuable during development, but now that totally removing debug intrinsics is coming up, this patch removes those options in favour of a single flag (experimental-debuginfo-iterators), which enables autoupgrade, in-memory debug records, and debug record printing to bitcode and textual IR. We need to do this ahead of removing the experimental-debuginfo-iterators flag, to reduce the amount of test-juggling that happens at that time. There are quite a number of weird test behaviours related to this -- some of which I simply delete in this test. Things like print-non-instruction-debug-info.ll , the test suite now checks for debug records in all tests, and we don't want to check we can print as intrinsics. Or the update_test_checks tests -- these are duplicated with write-experimental-debuginfo=false to ensure file writing for intrinsics is correct, but that's something we're imminently going to delete. A short survey of curious test changes: * free-intrinsics.ll: we don't need to test that debug-info is a zero cost intrinsic, because we won't be using intrinsics in the future. * undef-dbg-val.ll: apparently we pinned this to non-RemoveDIs in-memory mode while we sorted something out; it works now either way. * salvage-cast-debug-info.ll: was testing intrinsics-in-memory get salvaged, isn't necessary now * localize-constexpr-debuginfo.ll: was producing "dead metadata" intrinsics for optimised-out variable values, dbg-records takes the (correct) representation of poison/undef as an operand. Looks like we didn't update this in the past to avoid spurious test differences. * Transforms/Scalarizer/dbginfo.ll: this test was explicitly testing that debug-info affected codegen, and we deferred updating the tests until now. This is just one of those silent gnochange issues that get fixed by RemoveDIs. Finally: I've added a bitcode test, dbg-intrinsics-autoupgrade.ll.bc, that checks we can autoupgrade debug intrinsics that are in bitcode into the new debug records.
llvmbot
added
backend:X86
debuginfo
llvm:ir
llvm:analysis
|
@llvm/pr-subscribers-mlir-llvm @llvm/pr-subscribers-backend-x86 Author: Jeremy Morse (jmorse) ChangesDuring the transition from debug intrinsics to debug records, we used several different command line options to customise handling: the printing of debug records to bitcode and textual could be independent of how the debug-info was represented inside a module, whether the autoupgrader ran could be customised. This was all valuable during development, but now that totally removing debug intrinsics is coming up, this patch removes those options in favour of a single flag (experimental-debuginfo-iterators), which enables autoupgrade, in-memory debug records, and debug record printing to bitcode and textual IR. We need to do this ahead of removing the experimental-debuginfo-iterators flag, to reduce the amount of test-juggling that happens at that time. There are quite a number of weird test behaviours related to this -- some of which I simply delete in this test. Things like print-non-instruction-debug-info.ll , the test suite now checks for debug records in all tests, and we don't want to check we can print as intrinsics. Or the update_test_checks tests -- these are duplicated with write-experimental-debuginfo=false to ensure file writing for intrinsics is correct, but that's something we're imminently going to delete. A short survey of curious test changes:
Finally: I've added a bitcode test, dbg-intrinsics-autoupgrade.ll.bc, that checks we can autoupgrade debug intrinsics that are in bitcode into the new debug records. Patch is 171.24 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/131855.diff 46 Files Affected:
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index c0567090fdd2a..9eafff138e932 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -13407,7 +13407,7 @@ an extra level of indentation. As an example:
%inst2 = op2 %inst1, %c
These debug records replace the prior :ref:`debug intrinsics<dbg_intrinsics>`.
-Debug records will be disabled if ``--write-experimental-debuginfo=false`` is
+Debug records will be disabled if ``--experimental-debuginfo-iterators=false`` is
passed to LLVM; it is an error for both records and intrinsics to appear in the
same module. More information about debug records can be found in the `LLVM
Source Level Debugging <SourceLevelDebugging.html#format-common-intrinsics>`_
diff --git a/llvm/lib/AsmParser/LLParser.cpp b/llvm/lib/AsmParser/LLParser.cpp
index c8d792981793d..33c74e5ed8a88 100644
--- a/llvm/lib/AsmParser/LLParser.cpp
+++ b/llvm/lib/AsmParser/LLParser.cpp
@@ -65,8 +65,6 @@ static cl::opt<bool> AllowIncompleteIR(
extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;
-extern bool WriteNewDbgInfoFormatToBitcode;
-extern cl::opt<bool> WriteNewDbgInfoFormat;
static std::string getTypeString(Type *T) {
std::string Result;
@@ -213,8 +211,6 @@ bool LLParser::validateEndOfModule(bool UpgradeDebugInfo) {
"Mixed debug intrinsics/records seen without a parsing error?");
if (PreserveInputDbgFormat == cl::boolOrDefault::BOU_TRUE) {
UseNewDbgInfoFormat = SeenNewDbgInfoFormat;
- WriteNewDbgInfoFormatToBitcode = SeenNewDbgInfoFormat;
- WriteNewDbgInfoFormat = SeenNewDbgInfoFormat;
M->setNewDbgInfoFormatFlag(SeenNewDbgInfoFormat);
}
diff --git a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
index 40e755902b724..48458815ad6c2 100644
--- a/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
+++ b/llvm/lib/Bitcode/Reader/BitcodeReader.cpp
@@ -102,18 +102,8 @@ static cl::opt<bool> ExpandConstantExprs(
cl::desc(
"Expand constant expressions to instructions for testing purposes"));
-/// Load bitcode directly into RemoveDIs format (use debug records instead
-/// of debug intrinsics). UNSET is treated as FALSE, so the default action
-/// is to do nothing. Individual tools can override this to incrementally add
-/// support for the RemoveDIs format.
-cl::opt<cl::boolOrDefault> LoadBitcodeIntoNewDbgInfoFormat(
- "load-bitcode-into-experimental-debuginfo-iterators", cl::Hidden,
- cl::desc("Load bitcode directly into the new debug info format (regardless "
- "of input format)"));
extern cl::opt<bool> UseNewDbgInfoFormat;
extern cl::opt<cl::boolOrDefault> PreserveInputDbgFormat;
-extern bool WriteNewDbgInfoFormatToBitcode;
-extern cl::opt<bool> WriteNewDbgInfoFormat;
namespace {
@@ -4494,14 +4484,9 @@ Error BitcodeReader::parseGlobalIndirectSymbolRecord(
Error BitcodeReader::parseModule(uint64_t ResumeBit,
bool ShouldLazyLoadMetadata,
ParserCallbacks Callbacks) {
- // Load directly into RemoveDIs format if LoadBitcodeIntoNewDbgInfoFormat
- // has been set to true and we aren't attempting to preserve the existing
- // format in the bitcode (default action: load into the old debug format).
- if (PreserveInputDbgFormat != cl::boolOrDefault::BOU_TRUE) {
- TheModule->IsNewDbgInfoFormat =
- UseNewDbgInfoFormat &&
- LoadBitcodeIntoNewDbgInfoFormat != cl::boolOrDefault::BOU_FALSE;
- }
+ // In preparation for the deletion of debug-intrinsics, don't allow module
+ // loading to escape intrinsics being autoupgraded to debug records.
+ TheModule->IsNewDbgInfoFormat = UseNewDbgInfoFormat;
this->ValueTypeCallback = std::move(Callbacks.ValueType);
if (ResumeBit) {
@@ -7028,8 +7013,6 @@ Error BitcodeReader::materialize(GlobalValue *GV) {
SeenAnyDebugInfo ? SeenDebugRecord : F->getParent()->IsNewDbgInfoFormat;
if (SeenAnyDebugInfo) {
UseNewDbgInfoFormat = SeenDebugRecord;
- WriteNewDbgInfoFormatToBitcode = SeenDebugRecord;
- WriteNewDbgInfoFormat = SeenDebugRecord;
}
// If the module's debug info format doesn't match the observed input
// format, then set its format now; we don't need to call the conversion
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
index f6510a7a1549d..de6c3e64393b3 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriter.cpp
@@ -103,7 +103,6 @@ namespace llvm {
extern FunctionSummary::ForceSummaryHotnessType ForceSummaryEdgesCold;
}
-extern bool WriteNewDbgInfoFormatToBitcode;
extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
namespace {
@@ -3678,7 +3677,7 @@ void ModuleBitcodeWriter::writeFunction(
// they come after the instruction so that it's easy to attach them again
// when reading the bitcode, even though conceptually the debug locations
// start "before" the instruction.
- if (I.hasDbgRecords() && WriteNewDbgInfoFormatToBitcode) {
+ if (I.hasDbgRecords()) {
/// Try to push the value only (unwrapped), otherwise push the
/// metadata wrapped value. Returns true if the value was pushed
/// without the ValueAsMetadata wrapper.
diff --git a/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp b/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
index 5f66e1ea0a835..c33ac41b4f068 100644
--- a/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
+++ b/llvm/lib/Bitcode/Writer/BitcodeWriterPass.cpp
@@ -18,11 +18,8 @@
#include "llvm/Pass.h"
using namespace llvm;
-extern bool WriteNewDbgInfoFormatToBitcode;
-
PreservedAnalyses BitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
- ScopedDbgInfoFormatSetter FormatSetter(M, M.IsNewDbgInfoFormat &&
- WriteNewDbgInfoFormatToBitcode);
+ ScopedDbgInfoFormatSetter FormatSetter(M, M.IsNewDbgInfoFormat);
if (M.IsNewDbgInfoFormat)
M.removeDebugIntrinsicDeclarations();
@@ -55,7 +52,7 @@ namespace {
bool runOnModule(Module &M) override {
ScopedDbgInfoFormatSetter FormatSetter(
- M, M.IsNewDbgInfoFormat && WriteNewDbgInfoFormatToBitcode);
+ M, M.IsNewDbgInfoFormat);
if (M.IsNewDbgInfoFormat)
M.removeDebugIntrinsicDeclarations();
diff --git a/llvm/lib/CodeGen/MIRPrinter.cpp b/llvm/lib/CodeGen/MIRPrinter.cpp
index e9bd60e4e2597..2f08fcda1fbd0 100644
--- a/llvm/lib/CodeGen/MIRPrinter.cpp
+++ b/llvm/lib/CodeGen/MIRPrinter.cpp
@@ -69,7 +69,7 @@ static cl::opt<bool> SimplifyMIR(
static cl::opt<bool> PrintLocations("mir-debug-loc", cl::Hidden, cl::init(true),
cl::desc("Print MIR debug-locations"));
-extern cl::opt<bool> WriteNewDbgInfoFormat;
+extern cl::opt<bool> UseNewDbgInfoFormat;
namespace {
@@ -1050,7 +1050,7 @@ void MIRFormatter::printIRValue(raw_ostream &OS, const Value &V,
void llvm::printMIR(raw_ostream &OS, const Module &M) {
ScopedDbgInfoFormatSetter FormatSetter(const_cast<Module &>(M),
- WriteNewDbgInfoFormat);
+ UseNewDbgInfoFormat);
yaml::Output Out(OS);
Out << const_cast<Module &>(M);
@@ -1061,7 +1061,7 @@ void llvm::printMIR(raw_ostream &OS, const MachineModuleInfo &MMI,
// RemoveDIs: as there's no textual form for DbgRecords yet, print debug-info
// in dbg.value format.
ScopedDbgInfoFormatSetter FormatSetter(
- const_cast<Function &>(MF.getFunction()), WriteNewDbgInfoFormat);
+ const_cast<Function &>(MF.getFunction()), UseNewDbgInfoFormat);
MIRPrinter Printer(OS, MMI);
Printer.print(MF);
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index dca42a57fa9e3..d9ff0687480a4 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -44,11 +44,6 @@ cl::opt<cl::boolOrDefault> PreserveInputDbgFormat(
"contain debug records or intrinsics. Ignored in llvm-link, "
"llvm-lto, and llvm-lto2."));
-bool WriteNewDbgInfoFormatToBitcode /*set default value in cl::init() below*/;
-cl::opt<bool, true> WriteNewDbgInfoFormatToBitcode2(
- "write-experimental-debuginfo-iterators-to-bitcode", cl::Hidden,
- cl::location(WriteNewDbgInfoFormatToBitcode), cl::init(true));
-
DbgMarker *BasicBlock::createMarker(Instruction *I) {
assert(IsNewDbgInfoFormat &&
"Tried to create a marker in a non new debug-info block!");
diff --git a/llvm/lib/IR/IRPrintingPasses.cpp b/llvm/lib/IR/IRPrintingPasses.cpp
index 0dab0c9381635..96396d06ebba3 100644
--- a/llvm/lib/IR/IRPrintingPasses.cpp
+++ b/llvm/lib/IR/IRPrintingPasses.cpp
@@ -23,11 +23,7 @@
using namespace llvm;
-cl::opt<bool> WriteNewDbgInfoFormat(
- "write-experimental-debuginfo",
- cl::desc("Write debug info in the new non-intrinsic format. Has no effect "
- "if --preserve-input-debuginfo-format=true."),
- cl::init(true));
+extern cl::opt<bool> UseNewDbgInfoFormat;
namespace {
@@ -45,13 +41,11 @@ class PrintModulePassWrapper : public ModulePass {
ShouldPreserveUseListOrder(ShouldPreserveUseListOrder) {}
bool runOnModule(Module &M) override {
- // RemoveDIs: Regardless of the format we've processed this module in, use
- // `WriteNewDbgInfoFormat` to determine which format we use to write it.
- ScopedDbgInfoFormatSetter FormatSetter(M, WriteNewDbgInfoFormat);
+ ScopedDbgInfoFormatSetter FormatSetter(M, UseNewDbgInfoFormat);
// Remove intrinsic declarations when printing in the new format.
// TODO: Move this into Module::setIsNewDbgInfoFormat when we're ready to
// update test output.
- if (WriteNewDbgInfoFormat)
+ if (UseNewDbgInfoFormat)
M.removeDebugIntrinsicDeclarations();
if (llvm::isFunctionInPrintList("*")) {
@@ -93,9 +87,7 @@ class PrintFunctionPassWrapper : public FunctionPass {
// This pass just prints a banner followed by the function as it's processed.
bool runOnFunction(Function &F) override {
- // RemoveDIs: Regardless of the format we've processed this function in, use
- // `WriteNewDbgInfoFormat` to determine which format we use to write it.
- ScopedDbgInfoFormatSetter FormatSetter(F, WriteNewDbgInfoFormat);
+ ScopedDbgInfoFormatSetter FormatSetter(F, UseNewDbgInfoFormat);
if (isFunctionInPrintList(F.getName())) {
if (forcePrintModuleIR())
diff --git a/llvm/lib/IRPrinter/IRPrintingPasses.cpp b/llvm/lib/IRPrinter/IRPrintingPasses.cpp
index 026fa4d746d8b..e03e338c999e8 100644
--- a/llvm/lib/IRPrinter/IRPrintingPasses.cpp
+++ b/llvm/lib/IRPrinter/IRPrintingPasses.cpp
@@ -1,4 +1,4 @@
-//===--- IRPrintingPasses.cpp - Module and Function printing passes -------===//
+//===--- iRPrintingPasses.cpp - Module and Function printing passes -------===//
//
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information.
@@ -22,7 +22,7 @@
using namespace llvm;
-extern cl::opt<bool> WriteNewDbgInfoFormat;
+extern cl::opt<bool> UseNewDbgInfoFormat;
PrintModulePass::PrintModulePass() : OS(dbgs()) {}
PrintModulePass::PrintModulePass(raw_ostream &OS, const std::string &Banner,
@@ -33,13 +33,11 @@ PrintModulePass::PrintModulePass(raw_ostream &OS, const std::string &Banner,
EmitSummaryIndex(EmitSummaryIndex) {}
PreservedAnalyses PrintModulePass::run(Module &M, ModuleAnalysisManager &AM) {
- // RemoveDIs: Regardless of the format we've processed this module in, use
- // `WriteNewDbgInfoFormat` to determine which format we use to write it.
- ScopedDbgInfoFormatSetter FormatSetter(M, WriteNewDbgInfoFormat);
+ ScopedDbgInfoFormatSetter FormatSetter(M, UseNewDbgInfoFormat);
// Remove intrinsic declarations when printing in the new format.
// TODO: Move this into Module::setIsNewDbgInfoFormat when we're ready to
// update test output.
- if (WriteNewDbgInfoFormat)
+ if (UseNewDbgInfoFormat)
M.removeDebugIntrinsicDeclarations();
if (llvm::isFunctionInPrintList("*")) {
@@ -77,9 +75,7 @@ PrintFunctionPass::PrintFunctionPass(raw_ostream &OS, const std::string &Banner)
PreservedAnalyses PrintFunctionPass::run(Function &F,
FunctionAnalysisManager &) {
- // RemoveDIs: Regardless of the format we've processed this function in, use
- // `WriteNewDbgInfoFormat` to determine which format we use to write it.
- ScopedDbgInfoFormatSetter FormatSetter(F, WriteNewDbgInfoFormat);
+ ScopedDbgInfoFormatSetter FormatSetter(F, UseNewDbgInfoFormat);
if (isFunctionInPrintList(F.getName())) {
if (forcePrintModuleIR())
diff --git a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
index cd0e412bdf353..88abc6e560580 100644
--- a/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
+++ b/llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp
@@ -575,14 +575,13 @@ bool writeThinLTOBitcode(raw_ostream &OS, raw_ostream *ThinLinkOS,
}
} // anonymous namespace
-extern bool WriteNewDbgInfoFormatToBitcode;
+
PreservedAnalyses
llvm::ThinLTOBitcodeWriterPass::run(Module &M, ModuleAnalysisManager &AM) {
FunctionAnalysisManager &FAM =
AM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
- ScopedDbgInfoFormatSetter FormatSetter(M, M.IsNewDbgInfoFormat &&
- WriteNewDbgInfoFormatToBitcode);
+ ScopedDbgInfoFormatSetter FormatSetter(M, M.IsNewDbgInfoFormat);
if (M.IsNewDbgInfoFormat)
M.removeDebugIntrinsicDeclarations();
diff --git a/llvm/test/Analysis/CostModel/X86/free-intrinsics.ll b/llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
index 1e9fd0df78922..a8c5c43c3a9f8 100644
--- a/llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
+++ b/llvm/test/Analysis/CostModel/X86/free-intrinsics.ll
@@ -1,9 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
-;; Pin this test to not use "RemoveDIs" non-intrinsic debug-info. We get the
-;; correct output in that mode, but it generates spurious test changes, so
-;; avoid that for the moment.
-; RUN: opt -mtriple=x86_64-- -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=code-size %s -S -o - --experimental-debuginfo-iterators=false | FileCheck %s --check-prefix=CHECK-SIZE
-; RUN: opt -mtriple=x86_64-- -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=throughput %s -S -o - --experimental-debuginfo-iterators=false | FileCheck %s --check-prefix=CHECK-THROUGHPUT
+; RUN: opt -mtriple=x86_64-- -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=code-size %s -S -o - | FileCheck %s --check-prefix=CHECK-SIZE
+; RUN: opt -mtriple=x86_64-- -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=throughput %s -S -o - | FileCheck %s --check-prefix=CHECK-THROUGHPUT
define i32 @trivially_free() {
; CHECK-SIZE-LABEL: 'trivially_free'
@@ -11,9 +8,6 @@ define i32 @trivially_free() {
; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.assume(i1 undef)
; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.experimental.noalias.scope.decl(metadata !3)
; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.sideeffect()
-; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.dbg.assign(metadata ptr undef, metadata !6, metadata !DIExpression(), metadata !8, metadata ptr undef, metadata !DIExpression()), !dbg !9
-; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.dbg.declare(metadata ptr undef, metadata !6, metadata !DIExpression()), !dbg !9
-; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.dbg.label(metadata !10), !dbg !9
; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: %a1 = call ptr @llvm.invariant.start.p0(i64 1, ptr undef)
; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.invariant.end.p0(ptr undef, i64 1, ptr undef)
; CHECK-SIZE-NEXT: Cost Model: Found an estimated cost of 0 for instruction: %a2 = call ptr @llvm.launder.invariant.group.p0(ptr undef)
@@ -31,9 +25,6 @@ define i32 @trivially_free() {
; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.assume(i1 undef)
; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.experimental.noalias.scope.decl(metadata !3)
; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.sideeffect()
-; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.dbg.assign(metadata ptr undef, metadata !6, metadata !DIExpression(), metadata !8, metadata ptr undef, metadata !DIExpression()), !dbg !9
-; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.dbg.declare(metadata ptr undef, metadata !6, metadata !DIExpression()), !dbg !9
-; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.dbg.label(metadata !10), !dbg !9
; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: %a1 = call ptr @llvm.invariant.start.p0(i64 1, ptr undef)
; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: call void @llvm.invariant.end.p0(ptr undef, i64 1, ptr undef)
; CHECK-THROUGHPUT-NEXT: Cost Model: Found an estimated cost of 0 for instruction: %a2 = call ptr @llvm.launder.invariant.group.p0(ptr undef)
@@ -50,10 +41,6 @@ define i32 @trivially_free() {
call void @llvm.assume(i1 undef)
call void @llvm.experimental.noalias.scope.decl(metadata !4)
call void @llvm.sideeffect()
- call void @llvm.dbg.assign(metadata ptr undef, metadata !0, metadata !DIExpression(), metadata !10, metadata ptr undef, metadata !DIExpression()), !dbg !8
- call void @llvm.dbg.declare(metadata ptr undef, metadata !0, metadata !DIExpression()), !dbg !8
- call void @llvm.dbg.value(metadata i64 undef, i64 undef, metadata !DIExpression(), metadata !DIExpression()), !dbg !8
- call void @llvm.dbg.label(metadata !2), !dbg !8
%a1 = call ptr @llvm.invariant.start.p0(i64 1, ptr undef)
call void @llvm.invariant.end.p0(ptr undef, i64 1, ptr undef)
%a2 = call ptr @llvm.launder.invariant.group.p0(ptr undef)
@@ -71,10 +58,6 @@ declare i32 @llvm.annotation.i32(i32, ptr, ptr, i32)
declare void @llvm.assume(i1)
declare void @llvm.experimental.noalias.scope.decl(metadata)
declare void @llvm.sideeffect()
-declare void @llvm.dbg.assign(metadata, metadata, metadata, metadata, metadata, metadata)
-declare void @llvm.dbg.declare(metadata, metadata, metadata)
-declare void @llvm.dbg.value(metadata, i64, metadata, metadata)
-declare void @llvm.dbg.label(metadata)
declare ptr @llvm.invariant.start.p0(i64, ptr)
declare void @llvm.invariant.end.p0(ptr, i64, ptr)
declare ptr @llvm.launder.invariant.group.p0(ptr)
diff --git a/llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll b/llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
index 09fbd68d95e92..560af3d2b48fc 100644
--- a/llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
+++ b/llvm/test/Analysis/CostModel/free-intrinsics-datalayout.ll
@@ -1,9 +1,6 @@
; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py
-;; Pin this test to not use "RemoveDIs" non-intrinsic debug-info. We get the
-;; correct output in that mode, but it generates spurious test changes, so
-;; avoid that for the moment.
-; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=code-size %s -S -o - --experimental-debuginfo-iterators=false | FileCheck %s --check-prefix=CHECK-SIZE
-; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=throughput %s -S -o - --experimental-debuginfo-iterators=false | FileCheck %s --check-prefix=CHECK-THROUGHPUT
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=code-size %s -S -o - | FileCheck %s --check-prefix=CHECK-SIZE
+; RUN: opt -passes="print<cost-model>" 2>&1 -disable-output -cost-kind=throughput %s -S -o - | FileCheck %s --check-prefix=CHECK-THROUGHPUT
target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:6...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
OCHyams
approved these changes
Mar 20, 2025
There was a problem hiding this comment.
LGTM other than some nit/questions
Having a short amount of time where intrinsics are untested on main before they're removed seems fine/inevitable. Hmm, AFAICT we didn't put an llvm release timeline on the migration docs. I think we've broadcasted the plans in various places, I don't suppose you've got a link to anything like that, indicating that we're going to be removing the intrinsics? (If not, is it worth a discourse post, to remind people we always planned to be doing this and are doing it now?)
| ;; Test that we can read old debug intrinsics from bitcode, and autoupgrade | ||
| ;; them to the new debug-records format. | ||
| ; RUN: opt --passes=verify %s.bc -o - -S \ | ||
| ; RUN: | FileCheck %s --implicit-check-not=llvm.dbg |
There was a problem hiding this comment.
Is there a way to use llvm-bcanalyzer to check that the dbg.values are present in the bitcode, as insurance against this becoming rotten-green if the .bc gets changed at some point?
There was a problem hiding this comment.
Have added a bcanalyzer call that checks there are a series of CALL records interleaved with the other instructions in the function, I think that's sufficient for checking that intrinsics are present?
| ;; Test that we can read old debug intrinsics from bitcode, and autoupgrade | ||
| ;; them to the new debug-records format. | ||
| ; RUN: opt --passes=verify %s.bc -o - -S \ | ||
| ; RUN: | FileCheck %s --implicit-check-not=llvm.dbg | ||
|
|
There was a problem hiding this comment.
Do we also have an IR upgrade test?
There was a problem hiding this comment.
I've jammed it into this file too. IIRC it's technically not required to support past textual IR or something, but IMO it doesn't cost us anything.
| @@ -1,24 +0,0 @@ | |||
| ## Basic test checking that update_test_checks.py works correctly on various "IR value" kinds | |||
There was a problem hiding this comment.
Are we losing coverage deleting this one (without a dbg_records version)?
There was a problem hiding this comment.
There's an immediately adjacent various_ir_values_dbgrecords.test
|
For the public announcement, does https://discourse.llvm.org/t/psa-ir-output-changing-from-debug-intrinsics-to-debug-records/79578/3 qualify -- I think we've telegraphed fairly widely that intrinsics will go away, but there's only so much engagement we'll get. IMO after this patch lands we should quickly move to delete the "experimental-debuginfo-iterators" flag and request that someone reverts it if it's a blocker to them, that'll flush out anyone who still depends on intrinsics and will start a discussion on what's next. (Or: it'll reveal that no-one depends on it, and we can move further). |
|
The existing comms are probably enough. Maybe a bump wouldn't hurt, but I'll leave that up to you, as you say the removal commit is itself a bump. The plan SGTM |
Conflicts: llvm/tools/llvm-lto2/llvm-lto2.cpp Some options in llvm-lto2 were changed, that are adjacent to some of the lines I'm deleting that handle debug-info options.
|
Pushed up some feedback, merge to main (removes a spurious conflict), but also a patch to MLIR as there's some tested behaviour there to cope with dbg.values versus dbgrecords. @OCHyams could you give it a quick eyeball to confirm I'm not over-stepping here? I thing it's just mlir code to "do the stuff LLVM does", which now becomes redundant. |
OCHyams
reviewed
Mar 20, 2025
| WriteNewDbgInfoFormat); | ||
| if (WriteNewDbgInfoFormat) | ||
| llvmModule->removeDebugIntrinsicDeclarations(); | ||
| llvmModule->removeDebugIntrinsicDeclarations(); |
There was a problem hiding this comment.
Don't these removeDebugIntrinsicDeclarations calls (in both files) want to be guarded by if (UseNewDbgInfoFormat)?
I don't think it hugely matters if we're about to remove that flag too... but it looks like with your patch we unconditionally remove the intrinsic declarations, even if the intrinsic mode is selected?
There was a problem hiding this comment.
Switched to using UseNewDbgInfoFormat for the moment
|
Similar to the mlir test, I've switched a flang test to use I'll have another patch up at some point to remove all this test coverage in anticipation of all the RemoveDIs transition work being deleted. |
Ankur-0429
pushed a commit
to Ankur-0429/llvm-project
that referenced
this pull request
Apr 2, 2025
llvm#131855) During the transition from debug intrinsics to debug records, we used several different command line options to customise handling: the printing of debug records to bitcode and textual could be independent of how the debug-info was represented inside a module, whether the autoupgrader ran could be customised. This was all valuable during development, but now that totally removing debug intrinsics is coming up, this patch removes those options in favour of a single flag (experimental-debuginfo-iterators), which enables autoupgrade, in-memory debug records, and debug record printing to bitcode and textual IR. We need to do this ahead of removing the experimental-debuginfo-iterators flag, to reduce the amount of test-juggling that happens at that time. There are quite a number of weird test behaviours related to this -- some of which I simply delete in this commit. Things like print-non-instruction-debug-info.ll , the test suite now checks for debug records in all tests, and we don't want to check we can print as intrinsics. Or the update_test_checks tests -- these are duplicated with write-experimental-debuginfo=false to ensure file writing for intrinsics is correct, but that's something we're imminently going to delete. A short survey of curious test changes: * free-intrinsics.ll: we don't need to test that debug-info is a zero cost intrinsic, because we won't be using intrinsics in the future. * undef-dbg-val.ll: apparently we pinned this to non-RemoveDIs in-memory mode while we sorted something out; it works now either way. * salvage-cast-debug-info.ll: was testing intrinsics-in-memory get salvaged, isn't necessary now * localize-constexpr-debuginfo.ll: was producing "dead metadata" intrinsics for optimised-out variable values, dbg-records takes the (correct) representation of poison/undef as an operand. Looks like we didn't update this in the past to avoid spurious test differences. * Transforms/Scalarizer/dbginfo.ll: this test was explicitly testing that debug-info affected codegen, and we deferred updating the tests until now. This is just one of those silent gnochange issues that get fixed by RemoveDIs. Finally: I've added a bitcode test, dbg-intrinsics-autoupgrade.ll.bc, that checks we can autoupgrade debug intrinsics that are in bitcode into the new debug records.
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.
During the transition from debug intrinsics to debug records, we used several different command line options to customise handling: the printing of debug records to bitcode and textual could be independent of how the debug-info was represented inside a module, whether the autoupgrader ran could be customised. This was all valuable during development, but now that totally removing debug intrinsics is coming up, this patch removes those options in favour of a single flag (experimental-debuginfo-iterators), which enables autoupgrade, in-memory debug records, and debug record printing to bitcode and textual IR.
We need to do this ahead of removing the experimental-debuginfo-iterators flag, to reduce the amount of test-juggling that happens at that time.
There are quite a number of weird test behaviours related to this -- some of which I simply delete in this test. Things like print-non-instruction-debug-info.ll , the test suite now checks for debug records in all tests, and we don't want to check we can print as intrinsics. Or the update_test_checks tests -- these are duplicated with write-experimental-debuginfo=false to ensure file writing for intrinsics is correct, but that's something we're imminently going to delete.
A short survey of curious test changes:
Finally: I've added a bitcode test, dbg-intrinsics-autoupgrade.ll.bc, that checks we can autoupgrade debug intrinsics that are in bitcode into the new debug records.