[RemoveDIs] Add DPLabels support [3a/3] #82633
Merged
Conversation
This file contains 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
llvmbot
added
backend:PowerPC
debuginfo
llvm:globalisel
llvm:SelectionDAG
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-selectiondag Author: Orlando Cazalet-Hyams (OCHyams) ChangesPatch 2 of 3 to add llvm.dbg.label support to the RemoveDIs project. The AssignemntTrackingAnalysis support could have gone two ways:
I went with option 2 because it's less work and IMO 1 is no more correct conceptually. One thing to note is that this does sink the labels to the bottom of packs of debug records. e.g., [value, label, value] becomes [value, value, label]. This shouldn't be a problem because labels and variable locations don't have an ordering requirement. The ordering between variable locations is maintained and the label movement is deterministic. A self-hosted opt comes out identical with and without these labels enabled (next patch), fwiw. ("Enable dbg.label conversion" was originally part of 3, but that'll come separately in order to minimise the size of the capstone patch in case we need to revert it) I've added --try-experimental-debuginfo-iterators to affected tests - I figure this is still useful in case we need to un-enable RemoveDIs temporarily. Patch is 24.15 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/82633.diff 25 Files Affected:
diff --git a/llvm/include/llvm/IR/DebugProgramInstruction.h b/llvm/include/llvm/IR/DebugProgramInstruction.h
index 1c8619741eb69f..9b30b8330c4090 100644
--- a/llvm/include/llvm/IR/DebugProgramInstruction.h
+++ b/llvm/include/llvm/IR/DebugProgramInstruction.h
@@ -157,6 +157,11 @@ class DbgRecord : public ilist_node<DbgRecord> {
~DbgRecord() = default;
};
+inline raw_ostream &operator<<(raw_ostream &OS, const DbgRecord &R) {
+ R.print(OS);
+ return OS;
+}
+
/// Records a position in IR for a source label (DILabel). Corresponds to the
/// llvm.dbg.label intrinsic.
/// FIXME: Rename DbgLabelRecord when DPValue is renamed to DbgVariableRecord.
diff --git a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
index 7b66a851db2527..3b84624c3d4dca 100644
--- a/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
+++ b/llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
@@ -829,11 +829,7 @@ class MemLocFragmentFill {
void process(BasicBlock &BB, VarFragMap &LiveSet) {
BBInsertBeforeMap[&BB].clear();
for (auto &I : BB) {
- for (DbgRecord &DR : I.getDbgValueRange()) {
- // FIXME: DPValue::filter usage needs attention in this file; we need
- // to make sure dbg.labels are handled correctly in RemoveDIs mode.
- // Cast below to ensure this gets fixed when DPLabels are introduced.
- DPValue &DPV = cast<DPValue>(DR);
+ for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) {
if (const auto *Locs = FnVarLocs->getWedge(&DPV)) {
for (const VarLocInfo &Loc : *Locs) {
addDef(Loc, &DPV, *I.getParent(), LiveSet);
@@ -1919,6 +1915,9 @@ void AssignmentTrackingLowering::process(BasicBlock &BB, BlockInfo *LiveSet) {
// attached DPValues, or a non-debug instruction with attached unprocessed
// DPValues.
if (II != EI && II->hasDbgValues()) {
+ // Skip over non-variable debug records (i.e., labels). They're going to
+ // be read from IR (possibly re-ordering them within the debug record
+ // range) rather than from the analysis results.
for (DPValue &DPV : DPValue::filter(II->getDbgValueRange())) {
resetInsertionPoint(DPV);
processDPValue(DPV, LiveSet);
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index 7c95cef2eeb761..38bb808dd5bd53 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -3275,7 +3275,17 @@ void IRTranslator::translateDbgDeclareRecord(Value *Address, bool HasArgList,
void IRTranslator::translateDbgInfo(const Instruction &Inst,
MachineIRBuilder &MIRBuilder) {
- for (DPValue &DPV : DPValue::filter(Inst.getDbgValueRange())) {
+ for (DbgRecord &DR : Inst.getDbgValueRange()) {
+ if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) {
+ MIRBuilder.setDebugLoc(DPL->getDebugLoc());
+ assert(DPL->getLabel() && "Missing label");
+ assert(DPL->getLabel()->isValidLocationForIntrinsic(
+ MIRBuilder.getDebugLoc()) &&
+ "Expected inlined-at fields to agree");
+ MIRBuilder.buildDbgLabel(DPL->getLabel());
+ continue;
+ }
+ DPValue &DPV = cast<DPValue>(DR);
const DILocalVariable *Variable = DPV.getVariable();
const DIExpression *Expression = DPV.getExpression();
Value *V = DPV.getVariableLocationOp(0);
diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
index 5651498dd3f5aa..246762dd7ab628 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
@@ -1188,11 +1188,24 @@ void FastISel::handleDbgInfo(const Instruction *II) {
MIMD = MIMetadata();
// Reverse order of debug records, because fast-isel walks through backwards.
- for (DbgRecord &DPR : llvm::reverse(II->getDbgValueRange())) {
+ for (DbgRecord &DR : llvm::reverse(II->getDbgValueRange())) {
flushLocalValueMap();
recomputeInsertPt();
- DPValue &DPV = cast<DPValue>(DPR);
+ if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) {
+ assert(DPL->getLabel() && "Missing label");
+ if (!FuncInfo.MF->getMMI().hasDebugInfo()) {
+ LLVM_DEBUG(dbgs() << "Dropping debug info for " << *DPL << "\n");
+ continue;
+ }
+
+ BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DPL->getDebugLoc(),
+ TII.get(TargetOpcode::DBG_LABEL))
+ .addMetadata(DPL->getLabel());
+ continue;
+ }
+
+ DPValue &DPV = cast<DPValue>(DR);
Value *V = nullptr;
if (!DPV.hasArgList())
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index e893a5b616d33e..ee600d389c2cc3 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -1241,17 +1241,30 @@ void SelectionDAGBuilder::visitDbgInfo(const Instruction &I) {
It->Expr, Vals.size() > 1, It->DL, SDNodeOrder);
}
}
- // We must early-exit here to prevent any DPValues from being emitted below,
- // as we have just emitted the debug values resulting from assignment
- // tracking analysis, making any existing DPValues redundant (and probably
- // less correct).
- return;
}
+ // We must skip DPValues if they've already been processed above as we
+ // have just emitted the debug values resulting from assignment tracking
+ // analysis, making any existing DPValues redundant (and probably less
+ // correct). We still need to process DPLabels. This does sink DPLabels
+ // to the bottom of the group of debug records. That sholdn't be important
+ // as it does so deterministcally and ordering between DPLabels and DPValues
+ // is immaterial (other than for MIR/IR printing).
+ bool SkipDPValues = DAG.getFunctionVarLocs();
// Is there is any debug-info attached to this instruction, in the form of
- // DPValue non-instruction debug-info records.
- for (DbgRecord &DPR : I.getDbgValueRange()) {
- DPValue &DPV = cast<DPValue>(DPR);
+ // DbgRecord non-instruction debug-info records.
+ for (DbgRecord &DR : I.getDbgValueRange()) {
+ if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) {
+ assert(DPL->getLabel() && "Missing label");
+ SDDbgLabel *SDV =
+ DAG.getDbgLabel(DPL->getLabel(), DPL->getDebugLoc(), SDNodeOrder);
+ DAG.AddDbgLabel(SDV);
+ continue;
+ }
+
+ if (SkipDPValues)
+ continue;
+ DPValue &DPV = cast<DPValue>(DR);
DILocalVariable *Variable = DPV.getVariable();
DIExpression *Expression = DPV.getExpression();
dropDanglingDebugInfo(Variable, Expression);
diff --git a/llvm/lib/IR/AsmWriter.cpp b/llvm/lib/IR/AsmWriter.cpp
index c2a470c5fc7162..fba404c9b027cb 100644
--- a/llvm/lib/IR/AsmWriter.cpp
+++ b/llvm/lib/IR/AsmWriter.cpp
@@ -1141,12 +1141,14 @@ void SlotTracker::processFunctionMetadata(const Function &F) {
void SlotTracker::processDbgRecordMetadata(const DbgRecord &DR) {
if (const DPValue *DPV = dyn_cast<const DPValue>(&DR)) {
CreateMetadataSlot(DPV->getVariable());
- CreateMetadataSlot(DPV->getDebugLoc());
if (DPV->isDbgAssign())
CreateMetadataSlot(DPV->getAssignID());
+ } else if (const DPLabel *DPL = dyn_cast<const DPLabel>(&DR)) {
+ CreateMetadataSlot(DPL->getLabel());
} else {
llvm_unreachable("unsupported DbgRecord kind");
}
+ CreateMetadataSlot(DR.getDebugLoc());
}
void SlotTracker::processInstructionMetadata(const Instruction &I) {
diff --git a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
index f4f3070d11c7bb..4bf4b906299bd0 100644
--- a/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
+++ b/llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
@@ -293,6 +293,8 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
for (const auto &I : FromBlock) {
// Make note of any DPValues that need hoisting.
for (DbgRecord &DR : I.getDbgValueRange()) {
+ if (isa<DPLabel>(DR))
+ continue;
DPValue &DPV = cast<DPValue>(DR);
if (HasNoUnhoistedInstr(DPV.location_ops()))
DPValuesToHoist[DPV.getInstruction()].push_back(&DPV);
diff --git a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
index 7fd6759a61fbae..5bb109a04ff178 100644
--- a/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
+++ b/llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
@@ -386,7 +386,15 @@ static bool DPValuesRemoveRedundantDbgInstrsUsingBackwardScan(BasicBlock *BB) {
SmallVector<DPValue *, 8> ToBeRemoved;
SmallDenseSet<DebugVariable> VariableSet;
for (auto &I : reverse(*BB)) {
- for (DPValue &DPV : reverse(DPValue::filter(I.getDbgValueRange()))) {
+ for (DbgRecord &DR : reverse(I.getDbgValueRange())) {
+ if (isa<DPLabel>(DR)) {
+ // Emulate existing behaviour (see comment below for dbg.declares).
+ // FIXME: Don't do this.
+ VariableSet.clear();
+ continue;
+ }
+
+ DPValue &DPV = cast<DPValue>(DR);
// Skip declare-type records, as the debug intrinsic method only works
// on dbg.value intrinsics.
if (DPV.getType() == DPValue::LocationType::Declare) {
diff --git a/llvm/lib/Transforms/Utils/CodeExtractor.cpp b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
index 8ebcf0c04fd5a9..abcf3c30d2983b 100644
--- a/llvm/lib/Transforms/Utils/CodeExtractor.cpp
+++ b/llvm/lib/Transforms/Utils/CodeExtractor.cpp
@@ -1585,8 +1585,27 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
return cast<DILocalVariable>(NewVar);
};
- auto UpdateDPValuesOnInst = [&](Instruction &I) -> void {
- for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) {
+ auto UpdateDbgRecordsOnInst = [&](Instruction &I) -> void {
+ for (DbgRecord &DR : I.getDbgValueRange()) {
+ if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) {
+ // Point the intrinsic to a fresh label within the new function if the
+ // intrinsic was not inlined from some other function. Matches
+ // llvm.dbg.label intrinsic version in loop below.
+ if (DPL->getDebugLoc().getInlinedAt())
+ continue;
+ DILabel *OldLabel = DPL->getLabel();
+ DINode *&NewLabel = RemappedMetadata[OldLabel];
+ if (!NewLabel) {
+ DILocalScope *NewScope = DILocalScope::cloneScopeForSubprogram(
+ *OldLabel->getScope(), *NewSP, Ctx, Cache);
+ NewLabel = DILabel::get(Ctx, NewScope, OldLabel->getName(),
+ OldLabel->getFile(), OldLabel->getLine());
+ }
+ DPL->setLabel(cast<DILabel>(NewLabel));
+ continue;
+ }
+
+ DPValue &DPV = cast<DPValue>(DR);
// Apply the two updates that dbg.values get: invalid operands, and
// variable metadata fixup.
if (any_of(DPV.location_ops(), IsInvalidLocation)) {
@@ -1599,13 +1618,11 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
}
if (!DPV.getDebugLoc().getInlinedAt())
DPV.setVariable(GetUpdatedDIVariable(DPV.getVariable()));
- DPV.setDebugLoc(DebugLoc::replaceInlinedAtSubprogram(DPV.getDebugLoc(),
- *NewSP, Ctx, Cache));
}
};
for (Instruction &I : instructions(NewFunc)) {
- UpdateDPValuesOnInst(I);
+ UpdateDbgRecordsOnInst(I);
auto *DII = dyn_cast<DbgInfoIntrinsic>(&I);
if (!DII)
@@ -1658,6 +1675,9 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
if (const DebugLoc &DL = I.getDebugLoc())
I.setDebugLoc(
DebugLoc::replaceInlinedAtSubprogram(DL, *NewSP, Ctx, Cache));
+ for (DbgRecord &DR : I.getDbgValueRange())
+ DR.setDebugLoc(DebugLoc::replaceInlinedAtSubprogram(DR.getDebugLoc(),
+ *NewSP, Ctx, Cache));
// Loop info metadata may contain line locations. Fix them up.
auto updateLoopInfoLoc = [&Ctx, &Cache, NewSP](Metadata *MD) -> Metadata * {
diff --git a/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp b/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
index 08fdd3b75ffcbd..2ff7c015107677 100644
--- a/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
+++ b/llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
@@ -111,8 +111,7 @@ Instruction *getUntagLocationIfFunctionExit(Instruction &Inst) {
void StackInfoBuilder::visit(Instruction &Inst) {
// Visit non-intrinsic debug-info records attached to Inst.
- for (DbgRecord &DR : Inst.getDbgValueRange()) {
- DPValue &DPV = cast<DPValue>(DR);
+ for (DPValue &DPV : DPValue::filter(Inst.getDbgValueRange())) {
auto AddIfInteresting = [&](Value *V) {
if (auto *AI = dyn_cast_or_null<AllocaInst>(V)) {
if (!isInterestingAlloca(*AI))
diff --git a/llvm/lib/Transforms/Utils/ValueMapper.cpp b/llvm/lib/Transforms/Utils/ValueMapper.cpp
index 6e46469f5a601e..91ab2795a4b9d3 100644
--- a/llvm/lib/Transforms/Utils/ValueMapper.cpp
+++ b/llvm/lib/Transforms/Utils/ValueMapper.cpp
@@ -538,6 +538,11 @@ Value *Mapper::mapValue(const Value *V) {
}
void Mapper::remapDPValue(DbgRecord &DR) {
+ if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) {
+ DPL->setLabel(cast<DILabel>(mapMetadata(DPL->getLabel())));
+ return;
+ }
+
DPValue &V = cast<DPValue>(DR);
// Remap variables and DILocations.
auto *MappedVar = mapMetadata(V.getVariable());
diff --git a/llvm/test/CodeGen/Generic/live-debug-label.ll b/llvm/test/CodeGen/Generic/live-debug-label.ll
index 3121b8700ed119..095ec9931ceeb4 100644
--- a/llvm/test/CodeGen/Generic/live-debug-label.ll
+++ b/llvm/test/CodeGen/Generic/live-debug-label.ll
@@ -1,5 +1,6 @@
; RUN: llc < %s -stop-after=virtregrewriter -o - | FileCheck %s
-;
+; RUN: llc --try-experimental-debuginfo-iterators < %s -stop-after=virtregrewriter -o - | FileCheck %s
+
; NVPTX produces a different order of the BBs
; XFAIL: target=nvptx{{.*}}
; Both RISC-V and AMDGPU(GCN) deploy two VirtRegRewriter in their codegen
diff --git a/llvm/test/CodeGen/MIR/X86/branch-folder-with-label.mir b/llvm/test/CodeGen/MIR/X86/branch-folder-with-label.mir
index ce225d4567e91e..f7666fdcb26128 100644
--- a/llvm/test/CodeGen/MIR/X86/branch-folder-with-label.mir
+++ b/llvm/test/CodeGen/MIR/X86/branch-folder-with-label.mir
@@ -39,6 +39,7 @@
# }
#
# RUN: llc -o - %s -mtriple=x86_64-- -run-pass=branch-folder | FileCheck %s
+# RUN: llc --try-experimental-debuginfo-iterators -o - %s -mtriple=x86_64-- -run-pass=branch-folder | FileCheck %s
--- |
; ModuleID = 'test.ll'
source_filename = "test.ll"
diff --git a/llvm/test/CodeGen/PowerPC/debug-label-fast-isel.ll b/llvm/test/CodeGen/PowerPC/debug-label-fast-isel.ll
index 73848d829c5b2e..61f806cd309bbe 100644
--- a/llvm/test/CodeGen/PowerPC/debug-label-fast-isel.ll
+++ b/llvm/test/CodeGen/PowerPC/debug-label-fast-isel.ll
@@ -1,5 +1,5 @@
; RUN: llc < %s -mtriple powerpc64-ibm-aix-xcoff | FileCheck %s --check-prefix=CHECKASM
-
+; RUN: llc --try-experimental-debuginfo-iterators < %s -mtriple powerpc64-ibm-aix-xcoff | FileCheck %s --check-prefix=CHECKASM
; This is a case copied from test/DebugInfo/Generic/debug-label-mi.ll. This test
; is to explicitly check that fast isel for XCOFF works as expected for debug
; related intrinsics.
diff --git a/llvm/test/DebugInfo/Generic/PR37395.ll b/llvm/test/DebugInfo/Generic/PR37395.ll
index c2649392ff9c33..788500154279d3 100644
--- a/llvm/test/DebugInfo/Generic/PR37395.ll
+++ b/llvm/test/DebugInfo/Generic/PR37395.ll
@@ -1,4 +1,5 @@
; RUN: opt -passes=lcssa -S %s | FileCheck %s
+; RUN: opt --try-experimental-debuginfo-iterators -passes=lcssa -S %s | FileCheck %s
source_filename = "small.c"
@a = common dso_local global i32 0, align 4, !dbg !0
diff --git a/llvm/test/DebugInfo/Generic/debug-label-inline.ll b/llvm/test/DebugInfo/Generic/debug-label-inline.ll
index 26407cc8f702a8..0342b0af748c5a 100644
--- a/llvm/test/DebugInfo/Generic/debug-label-inline.ll
+++ b/llvm/test/DebugInfo/Generic/debug-label-inline.ll
@@ -1,4 +1,5 @@
; RUN: llc -O0 -filetype=obj -o - %s | llvm-dwarfdump -v - | FileCheck %s
+; RUN: llc --try-experimental-debuginfo-iterators -O0 -filetype=obj -o - %s | llvm-dwarfdump -v - | FileCheck %s
;
; Bug 47129
; XFAIL: target=sparc{{.*}}
diff --git a/llvm/test/DebugInfo/Generic/debug-label-mi.ll b/llvm/test/DebugInfo/Generic/debug-label-mi.ll
index ad92dd2f5a7c29..b7086bcc7c3e91 100644
--- a/llvm/test/DebugInfo/Generic/debug-label-mi.ll
+++ b/llvm/test/DebugInfo/Generic/debug-label-mi.ll
@@ -2,11 +2,14 @@
; REQUIRES: asserts
; RUN: llc -debug-only=isel %s -o /dev/null 2> %t.debug
; RUN: cat %t.debug | FileCheck %s --check-prefix=CHECKMI
-;
+; RUN: llc --try-experimental-debuginfo-iterators -debug-only=isel %s -o /dev/null 2> %t.debug
+; RUN: cat %t.debug | FileCheck %s --check-prefix=CHECKMI
+
; CHECKMI: DBG_LABEL "top", debug-location !9
; CHECKMI: DBG_LABEL "done", debug-location !11
;
; RUN: llc %s -o - | FileCheck %s --check-prefix=CHECKASM
+; RUN: llc --try-experimental-debuginfo-iterators %s -o - | FileCheck %s --check-prefix=CHECKASM
;
; CHECKASM: DEBUG_LABEL: foo:top
; CHECKASM: DEBUG_LABEL: foo:done
diff --git a/llvm/test/DebugInfo/Generic/debug-label-opt.ll b/llvm/test/DebugInfo/Generic/debug-label-opt.ll
index e875216164305a..8426f696c5c60f 100644
--- a/llvm/test/DebugInfo/Generic/debug-label-opt.ll
+++ b/llvm/test/DebugInfo/Generic/debug-label-opt.ll
@@ -3,6 +3,12 @@
; REQUIRES: asserts
; RUN: llc -debug-only=isel %s -o /dev/null 2> %t.debug
; RUN: cat %t.debug | FileCheck %s --check-prefix=CHECKMI
+;; FIXME: RemoveDIs debug output is slightly different, e.g.:
+;; - DBG_LABEL "top", debug-location !9; debug-label-mi.c:4:1
+;; + DBG_LABEL "top", debug-location !DILocation(line: 4, column: 1, scope: !4); debug-label-mi.c:4:1
+;; Disable these run lines until that is understood/fixed.
+; run: llc --try-experimental-debuginfo-iterators -debug-only=isel %s -o /dev/null 2> %t.debug
+; run: cat %t.debug | FileCheck %s --check-prefix=CHECKMI
;
; CHECKMI: DBG_LABEL "end_sum", debug-location !17
; CHECKMI: DBG_LABEL "end", debug-location !19
diff --git a/llvm/test/DebugInfo/Generic/debug-label.ll b/llvm/test/DebugInfo/Generic/debug-label.ll
index eff482a25ee0fe..fbe0f975bf33ad 100644
--- a/llvm/test/DebugInfo/Generic/debug-label.ll
+++ b/llvm/test/DebugInfo/Generic/debug-label.ll
@@ -1,5 +1,6 @@
; RUN: llc -O0 -filetype=obj -o - %s | llvm-dwarfdump -v - | FileCheck %s
-;
+; RUN: llc --try-experimental-debuginfo-iterators -O0 -filetype=obj -o - %s | llvm-dwarfdump -v - | FileCheck %s
+
; CHECK: .debug_info contents:
; CHECK: DW_TAG_label
; CHECK-NEXT: DW_AT_name {{.*}}"top"
@@ -14,6 +15,7 @@
; CHECK-NOT: DW_AT_name {{.*}}"top"
;
; RUN: llc -O0 -o - %s | FileCheck %s -check-prefix=ASM
+; RUN: llc --try-experimental-debuginfo-iterators -O0 -o - %s | FileCheck %s -check-prefix=ASM
;
; ASM: [[TOP_LOW_PC:[.0-9a-zA-Z]+]]:{{[[:space:]].*}}DEBUG_LABEL: foo:top
; ASM: [[DONE_LOW_PC:[.0-9a-zA-Z]+]]:{{[[:space:]].*}}DEBUG_LABEL: foo:done
diff --git a/llvm/test/DebugInfo/X86/debug-label-unreached.ll b/llvm/test/DebugInfo/X86/debug-label-unreached.ll
index b84fb07d93ea61..f4d4acaa71153f 100644
--- a/llvm/test/DebugInfo/X86/debug-label-unreached.ll
+++ b/llvm/test/DebugInfo/X86/debug-label-unreached.ll
@@ -1,6 +1,7 @@
; Test unreachable llvm.dbg.label
;
; RUN: llc -filetype=obj -split-dwarf-file debug.dwo -mtriple=x86_64-unknown-linux-gnu -o - %s | llvm-dwarfdump -v - | FileCheck %s
+; RUN: llc --try-experimental-debuginfo-iterators -filetype=obj -split-dwarf-file debug.dwo -mtriple=x86_64-unknown-linux-gnu -o - %s | llvm-dwarfdump -v - | FileCheck %s
;
; CHECK: .debug_info.dwo contents:
; CHECK: DW_TAG_label
diff --git a/llvm/test/DebugInfo/X86/live-debug-vars-nodebug.ll b/llvm/test/DebugInfo/X86/live-debug-vars-nodebug.ll
index 35d05392209dbf..334a6e5f4c5e88 100644
--- a/llvm/test/DebugInfo/X86/live-debug-vars-nodebug.ll
+++ b/llvm/test/DebugInfo/X86/live-debug-vars-nodebug.ll
@@ -5,6 +5,10 @@
; RUN: -experimental-debug-variable-locations \
; RUN: | FileCheck %s --check-prefix=EXPER-INPUT
+; RUN: llc %s -mtriple=x86_64-unknown-unknown -o - -stop-before=finalize-isel \
+; RUN: --try-experimental-debuginfo-iterators \
+; RUN: | FileCheck %s --check-prefix=EXPER-INPUT
+
; R...
[truncated]
|
OCHyams
changed the title
SLTozer
reviewed
Feb 22, 2024
Comment on lines
295
to
297
| for (DbgRecord &DR : I.getDbgValueRange()) { | ||
| if (isa<DPLabel>(DR)) | ||
| continue; |
There was a problem hiding this comment.
Suggested change
| for (DbgRecord &DR : I.getDbgValueRange()) { | |
| if (isa<DPLabel>(DR)) | |
| continue; | |
| for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) { |
| for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) { | ||
| auto UpdateDbgRecordsOnInst = [&](Instruction &I) -> void { | ||
| for (DbgRecord &DR : I.getDbgValueRange()) { | ||
| if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) { |
There was a problem hiding this comment.
Would this work with an auto lambda to share the logic for DbgLabelInsts?
There was a problem hiding this comment.
Possibly - the DPValue code here hasn't been auto-lambda'd. I'll have a play with it.
There was a problem hiding this comment.
Yeah that looks better, done (added a setLabel method to DbgLabelInst (intrinsic version)).
| @@ -1,5 +1,6 @@ | |||
| ; RUN: llc < %s -stop-after=virtregrewriter -o - | FileCheck %s | |||
| ; | |||
| ; RUN: llc --try-experimental-debuginfo-iterators < %s -stop-after=virtregrewriter -o - | FileCheck %s | |||
There was a problem hiding this comment.
Do this and the other test updates need to be done now that RemoveDIs is on by default? If we do keep this, I think we'd need one with --experimental-debuginfo-iterators=false and one with true to ensure we're covering both paths.
There was a problem hiding this comment.
I mention this in the patch summary - my thinking is that this preserves coverage if we need to disable RemoveDIs temporarily. Roping in @jmorse to see if he's got an opinion on testing strategy going forward.
I think we'd need one with --experimental-debuginfo-iterators=false
However, I don't think this question (above) is for this patch, as that could arguably be applied to all tests with debug info?
There was a problem hiding this comment.
Yeah, it doesn't necessarily need to be added here - the important part is just that adding --try-experimental-debuginfo-iterators is unnecessary/meaningless now, since we're already using it by default.
There was a problem hiding this comment.
The (offline) consensus agrees with your position -- I've removed them from the patch.
| @@ -3,6 +3,12 @@ | |||
| ; REQUIRES: asserts | |||
| ; RUN: llc -debug-only=isel %s -o /dev/null 2> %t.debug | |||
| ; RUN: cat %t.debug | FileCheck %s --check-prefix=CHECKMI | |||
| ;; FIXME: RemoveDIs debug output is slightly different, e.g.: | |||
There was a problem hiding this comment.
Nope, I must've dropped a commit that removed this comment...
OCHyams
commented
Feb 22, 2024
| for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) { | ||
| auto UpdateDbgRecordsOnInst = [&](Instruction &I) -> void { | ||
| for (DbgRecord &DR : I.getDbgValueRange()) { | ||
| if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) { |
There was a problem hiding this comment.
Possibly - the DPValue code here hasn't been auto-lambda'd. I'll have a play with it.
| @@ -1,5 +1,6 @@ | |||
| ; RUN: llc < %s -stop-after=virtregrewriter -o - | FileCheck %s | |||
| ; | |||
| ; RUN: llc --try-experimental-debuginfo-iterators < %s -stop-after=virtregrewriter -o - | FileCheck %s | |||
There was a problem hiding this comment.
I mention this in the patch summary - my thinking is that this preserves coverage if we need to disable RemoveDIs temporarily. Roping in @jmorse to see if he's got an opinion on testing strategy going forward.
I think we'd need one with --experimental-debuginfo-iterators=false
However, I don't think this question (above) is for this patch, as that could arguably be applied to all tests with debug info?
| @@ -3,6 +3,12 @@ | |||
| ; REQUIRES: asserts | |||
| ; RUN: llc -debug-only=isel %s -o /dev/null 2> %t.debug | |||
| ; RUN: cat %t.debug | FileCheck %s --check-prefix=CHECKMI | |||
| ;; FIXME: RemoveDIs debug output is slightly different, e.g.: | |||
There was a problem hiding this comment.
Nope, I must've dropped a commit that removed this comment...
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SLTozer
reviewed
Feb 23, 2024
Comment on lines
+160
to
+164
| inline raw_ostream &operator<<(raw_ostream &OS, const DbgRecord &R) { | ||
| R.print(OS); | ||
| return OS; | ||
| } | ||
|
|
There was a problem hiding this comment.
One more thing, since DbgRecord::print already dispatches, we could simply replace the DPValue operator with this, right?
SLTozer
reviewed
Feb 23, 2024
SLTozer
approved these changes
Feb 23, 2024
|
Removed --try-experimental-debuginfo-iterators from tests. For posterity, this patch stimulates the following tests: |
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.
Patch 2 of 3 to add llvm.dbg.label support to the RemoveDIs project. The
patch stack adds the DPLabel class, which is the RemoveDIs llvm.dbg.label
equivalent.
AssignemntTrackingAnalysis support could have gone two ways:
I went with option 2 because it's less work and IMO 1 is no more correct conceptually. One thing to note is that this does sink the labels to the bottom of packs of debug records. e.g., [value, label, value] becomes [value, value, label]. This shouldn't be a problem because labels and variable locations don't have an ordering requirement. The ordering between variable locations is maintained and the label movement is deterministic. A self-hosted opt comes out identical with and without these labels enabled (next patch), fwiw.
("Enable dbg.label conversion" was originally part of 3, but that'll come separately in order to minimise the size of the capstone patch in case we need to revert it)
I've added --try-experimental-debuginfo-iterators to affected tests - I figure this is still useful in case we need to un-enable RemoveDIs temporarily.