Skip to content

[RemoveDIs] Add DPLabels support [3a/3] #82633

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Feb 23, 2024
Merged
10 changes: 5 additions & 5 deletions llvm/include/llvm/IR/DebugProgramInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Comment on lines +160 to +164
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing, since DbgRecord::print already dispatches, we could simply replace the DPValue operator with this, right?

/// 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.
Expand Down Expand Up @@ -536,11 +541,6 @@ inline raw_ostream &operator<<(raw_ostream &OS, const DPMarker &Marker) {
return OS;
}

inline raw_ostream &operator<<(raw_ostream &OS, const DPValue &Value) {
Value.print(OS);
return OS;
}

/// Inline helper to return a range of DPValues attached to a marker. It needs
/// to be inlined as it's frequently called, but also come after the declaration
/// of DPMarker. Thus: it's pre-declared by users like Instruction, then an
Expand Down
3 changes: 3 additions & 0 deletions llvm/include/llvm/IR/IntrinsicInst.h
Original file line number Diff line number Diff line change
Expand Up @@ -531,6 +531,9 @@ class DbgAssignIntrinsic : public DbgValueInst {
class DbgLabelInst : public DbgInfoIntrinsic {
public:
DILabel *getLabel() const { return cast<DILabel>(getRawLabel()); }
void setLabel(DILabel *NewLabel) {
setArgOperand(0, MetadataAsValue::get(getContext(), NewLabel));
}

Metadata *getRawLabel() const {
return cast<MetadataAsValue>(getArgOperand(0))->getMetadata();
Expand Down
9 changes: 4 additions & 5 deletions llvm/lib/CodeGen/AssignmentTrackingAnalysis.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 11 additions & 1 deletion llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
17 changes: 15 additions & 2 deletions llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
29 changes: 21 additions & 8 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion llvm/lib/IR/AsmWriter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
6 changes: 3 additions & 3 deletions llvm/lib/Transforms/Scalar/SpeculativeExecution.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -291,9 +291,9 @@ bool SpeculativeExecutionPass::considerHoistingFromTo(
InstructionCost TotalSpeculationCost = 0;
unsigned NotHoistedInstCount = 0;
for (const auto &I : FromBlock) {
// Make note of any DPValues that need hoisting.
for (DbgRecord &DR : I.getDbgValueRange()) {
DPValue &DPV = cast<DPValue>(DR);
// Make note of any DPValues that need hoisting. DPLabels
// get left behind just like llvm.dbg.labels.
for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) {
if (HasNoUnhoistedInstr(DPV.location_ops()))
DPValuesToHoist[DPV.getInstruction()].push_back(&DPV);
}
Expand Down
10 changes: 9 additions & 1 deletion llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
45 changes: 29 additions & 16 deletions llvm/lib/Transforms/Utils/CodeExtractor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1585,8 +1585,30 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
return cast<DILocalVariable>(NewVar);
};

auto UpdateDPValuesOnInst = [&](Instruction &I) -> void {
for (DPValue &DPV : DPValue::filter(I.getDbgValueRange())) {
auto UpdateDbgLabel = [&](auto *LabelRecord) {
// Point the label record to a fresh label within the new function if
// the record was not inlined from some other function.
if (LabelRecord->getDebugLoc().getInlinedAt())
return;
DILabel *OldLabel = LabelRecord->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());
}
LabelRecord->setLabel(cast<DILabel>(NewLabel));
};

auto UpdateDbgRecordsOnInst = [&](Instruction &I) -> void {
for (DbgRecord &DR : I.getDbgValueRange()) {
if (DPLabel *DPL = dyn_cast<DPLabel>(&DR)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this work with an auto lambda to share the logic for DbgLabelInsts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly - the DPValue code here hasn't been auto-lambda'd. I'll have a play with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that looks better, done (added a setLabel method to DbgLabelInst (intrinsic version)).

UpdateDbgLabel(DPL);
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)) {
Expand All @@ -1599,13 +1621,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)
Expand All @@ -1614,17 +1634,7 @@ static void fixupDebugInfoPostExtraction(Function &OldFunc, Function &NewFunc,
// Point the intrinsic to a fresh label within the new function if the
// intrinsic was not inlined from some other function.
if (auto *DLI = dyn_cast<DbgLabelInst>(&I)) {
if (DLI->getDebugLoc().getInlinedAt())
continue;
DILabel *OldLabel = DLI->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());
}
DLI->setArgOperand(0, MetadataAsValue::get(Ctx, NewLabel));
UpdateDbgLabel(DLI);
continue;
}

Expand Down Expand Up @@ -1658,6 +1668,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 * {
Expand Down
3 changes: 1 addition & 2 deletions llvm/lib/Transforms/Utils/MemoryTaggingSupport.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
5 changes: 5 additions & 0 deletions llvm/lib/Transforms/Utils/ValueMapper.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down
5 changes: 5 additions & 0 deletions llvm/test/Transforms/SpeculativeExecution/PR46267.ll
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,16 @@ land.rhs: ; preds = %entry
; CHECK-NEXT: call void @llvm.dbg.declare(metadata ptr %y
; CHECK-NEXT: %a0 = load i32, ptr undef, align 1
; CHECK-NEXT: call void @llvm.dbg.value(metadata i32 %a0
; CHECK-NEXT: call void @llvm.dbg.label
call void @llvm.dbg.label(metadata !11), !dbg !10
%y = alloca i32, align 4
call void @llvm.dbg.declare(metadata ptr %y, metadata !14, metadata !DIExpression()), !dbg !10

%a0 = load i32, ptr undef, align 1
call void @llvm.dbg.value(metadata i32 %a0, metadata !9, metadata !DIExpression()), !dbg !10
;; RemoveDIs: Check a label that is attached to a hoisted instruction
;; gets left behind (match intrinsic-style debug info behaviour).
call void @llvm.dbg.label(metadata !15), !dbg !10

%a2 = add i32 %i, 0
call void @llvm.dbg.value(metadata i32 %a2, metadata !13, metadata !DIExpression()), !dbg !10
Expand Down Expand Up @@ -82,3 +86,4 @@ attributes #1 = { nounwind readnone speculatable willreturn }
!12 = !DILocalVariable(name: "x", scope: !6, file: !1, line: 3, type: !4)
!13 = !DILocalVariable(name: "a2", scope: !6, file: !1, line: 3, type: !4)
!14 = !DILocalVariable(name: "y", scope: !6, file: !1, line: 3, type: !4)
!15 = !DILabel(scope: !6, name: "label2", file: !1, line: 2)