Skip to content

[RemoveDIs] Update DIBuilder to conditionally insert DbgRecords #84739

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.

Sign up for GitHub

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 36 commits into from
Mar 12, 2024
Merged

[RemoveDIs] Update DIBuilder to conditionally insert DbgRecords #84739

Show file tree
Hide file tree
Changes from 33 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
b25c546
Add DbgInstPtr union, update labels
OCHyams Mar 4, 2024
b503e84
Add insertLabel body
OCHyams Mar 4, 2024
3964cc5
labels test try#1
OCHyams Mar 4, 2024
36c7aa0
continue to work on labeles test
OCHyams Mar 4, 2024
a706ca6
flushTerminators after every new instruction is inserted -- possibly …
OCHyams Mar 4, 2024
c3cec69
do what that commit says
OCHyams Mar 4, 2024
f443a18
Allow insert-before at end() and test it in unittest -- needed for clang
OCHyams Mar 4, 2024
88f6e21
tidy up the label test
OCHyams Mar 4, 2024
c6bbfe4
dbg.values WIP: mem2reg and C API need attendance
OCHyams Mar 4, 2024
035630f
add unittest for dbg.vals
OCHyams Mar 5, 2024
1f95a01
declare API. Sans C-API. Needs test and impl
OCHyams Mar 5, 2024
eb428e6
add insertDPValue helper
OCHyams Mar 5, 2024
42f4e74
declare impl + tidy some others
OCHyams Mar 5, 2024
063f9cd
dbg.declare unittest
OCHyams Mar 5, 2024
60f9a2f
dbg.assign api and impl
OCHyams Mar 5, 2024
3a15cfa
assign test -not working
OCHyams Mar 5, 2024
7754eb9
fix dbg.assign test
OCHyams Mar 5, 2024
0a96251
insert dpassigns 'after' in DIBuilder
OCHyams Mar 7, 2024
44008e2
gross C++
OCHyams Mar 7, 2024
2e69a99
C-API approach #1 - return a union
OCHyams Mar 8, 2024
8b03c20
Revert "C-API approach #1 - return a union"
OCHyams Mar 8, 2024
63511e0
other C-API tactic: use 2 function falvours
OCHyams Mar 8, 2024
197ab57
add C API info to the docs
OCHyams Mar 8, 2024
ef7ce1a
allow empty to empty splice with trailing dpvalues, fixes OpenMP/canc…
OCHyams Mar 9, 2024
333bc9e
Fix clang/test/Frontend/stack-layout-remark.c
OCHyams Mar 10, 2024
5cc4a3e
format
OCHyams Mar 10, 2024
4e0bfff
tidy
OCHyams Mar 10, 2024
905a794
fix comment
OCHyams Mar 10, 2024
a7bbba3
fix leftover dev comment/code
OCHyams Mar 10, 2024
edecd14
undo uneeded change
OCHyams Mar 10, 2024
25c66b1
Remove C-API changes
OCHyams Mar 10, 2024
04a4e29
undo over-excited clang-format
OCHyams Mar 10, 2024
e12c7dc
remove sneaky leftover
OCHyams Mar 10, 2024
bf1c8c1
fix comments and assertions
OCHyams Mar 11, 2024
8ea9e72
SROA suggestion
OCHyams Mar 11, 2024
ba210ae
undo assertion change
OCHyams Mar 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
73 changes: 39 additions & 34 deletions llvm/include/llvm/IR/DIBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ namespace llvm {
class Module;
class Value;
class DbgAssignIntrinsic;
class DbgRecord;

using DbgInstPtr = PointerUnion<Instruction *, DbgRecord *>;

class DIBuilder {
Module &M;
Expand Down Expand Up @@ -90,13 +93,17 @@ namespace llvm {
void trackIfUnresolved(MDNode *N);

/// Internal helper for insertDeclare.
Instruction *insertDeclare(llvm::Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertBB, Instruction *InsertBefore);
DbgInstPtr insertDeclare(llvm::Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertBB, Instruction *InsertBefore);

/// Internal helper for insertLabel.
Instruction *insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertBB, Instruction *InsertBefore);
DbgInstPtr insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertBB, Instruction *InsertBefore);

/// Internal helper. Track metadata if untracked and insert \p DPV.
void insertDPValue(DPValue *DPV, BasicBlock *InsertBB,
Instruction *InsertBefore, bool InsertAtHead = false);

/// Internal helper with common code used by insertDbg{Value,Addr}Intrinsic.
Instruction *insertDbgIntrinsic(llvm::Function *Intrinsic, llvm::Value *Val,
Expand All @@ -106,10 +113,11 @@ namespace llvm {
Instruction *InsertBefore);

/// Internal helper for insertDbgValueIntrinsic.
Instruction *
insertDbgValueIntrinsic(llvm::Value *Val, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertBB, Instruction *InsertBefore);
DbgInstPtr insertDbgValueIntrinsic(llvm::Value *Val,
DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertBB,
Instruction *InsertBefore);

public:
/// Construct a builder for a module.
Expand Down Expand Up @@ -921,9 +929,9 @@ namespace llvm {
/// \param Expr A complex location expression.
/// \param DL Debug info location.
/// \param InsertAtEnd Location for the new intrinsic.
Instruction *insertDeclare(llvm::Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertAtEnd);
DbgInstPtr insertDeclare(llvm::Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertAtEnd);

/// Insert a new llvm.dbg.assign intrinsic call.
/// \param LinkedInstr Instruction with a DIAssignID to link with the new
Expand All @@ -939,59 +947,56 @@ namespace llvm {
/// \param DL Debug info location, usually: (line: 0,
/// column: 0, scope: var-decl-scope). See
/// getDebugValueLoc.
DbgAssignIntrinsic *insertDbgAssign(Instruction *LinkedInstr, Value *Val,
DILocalVariable *SrcVar,
DIExpression *ValExpr, Value *Addr,
DIExpression *AddrExpr,
const DILocation *DL);
DbgInstPtr insertDbgAssign(Instruction *LinkedInstr, Value *Val,
DILocalVariable *SrcVar, DIExpression *ValExpr,
Value *Addr, DIExpression *AddrExpr,
const DILocation *DL);

/// Insert a new llvm.dbg.declare intrinsic call.
/// \param Storage llvm::Value of the variable
/// \param VarInfo Variable's debug info descriptor.
/// \param Expr A complex location expression.
/// \param DL Debug info location.
/// \param InsertBefore Location for the new intrinsic.
Instruction *insertDeclare(llvm::Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
Instruction *InsertBefore);
DbgInstPtr insertDeclare(llvm::Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
Instruction *InsertBefore);

/// Insert a new llvm.dbg.label intrinsic call.
/// \param LabelInfo Label's debug info descriptor.
/// \param DL Debug info location.
/// \param InsertBefore Location for the new intrinsic.
Instruction *insertLabel(DILabel *LabelInfo, const DILocation *DL,
Instruction *InsertBefore);
DbgInstPtr insertLabel(DILabel *LabelInfo, const DILocation *DL,
Instruction *InsertBefore);

/// Insert a new llvm.dbg.label intrinsic call.
/// \param LabelInfo Label's debug info descriptor.
/// \param DL Debug info location.
/// \param InsertAtEnd Location for the new intrinsic.
Instruction *insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertAtEnd);
DbgInstPtr insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertAtEnd);

/// Insert a new llvm.dbg.value intrinsic call.
/// \param Val llvm::Value of the variable
/// \param VarInfo Variable's debug info descriptor.
/// \param Expr A complex location expression.
/// \param DL Debug info location.
/// \param InsertAtEnd Location for the new intrinsic.
Instruction *insertDbgValueIntrinsic(llvm::Value *Val,
DILocalVariable *VarInfo,
DIExpression *Expr,
const DILocation *DL,
BasicBlock *InsertAtEnd);
DbgInstPtr insertDbgValueIntrinsic(llvm::Value *Val,
DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertAtEnd);

/// Insert a new llvm.dbg.value intrinsic call.
/// \param Val llvm::Value of the variable
/// \param VarInfo Variable's debug info descriptor.
/// \param Expr A complex location expression.
/// \param DL Debug info location.
/// \param InsertBefore Location for the new intrinsic.
Instruction *insertDbgValueIntrinsic(llvm::Value *Val,
DILocalVariable *VarInfo,
DIExpression *Expr,
const DILocation *DL,
Instruction *InsertBefore);
DbgInstPtr insertDbgValueIntrinsic(llvm::Value *Val,
DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
Instruction *InsertBefore);

/// Replace the vtable holder in the given type.
///
Expand Down
13 changes: 3 additions & 10 deletions llvm/lib/IR/BasicBlock.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,8 +752,6 @@ void BasicBlock::spliceDebugInfoEmptyBlock(BasicBlock::iterator Dest,
// occur when a block is optimised away and the terminator has been moved
// somewhere else.
if (Src->empty()) {
assert(Dest != end() &&
"Transferring trailing DPValues to another trailing position");
Comment on lines -755 to -756
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this something that we see happen when building BBs in clang?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is hit by test OpenMP/cancel_codegen.cpp

DPMarker *SrcTrailingDPValues = Src->getTrailingDPValues();
if (!SrcTrailingDPValues)
return;
Expand Down Expand Up @@ -1038,15 +1036,10 @@ void BasicBlock::insertDPValueAfter(DbgRecord *DPV, Instruction *I) {

void BasicBlock::insertDPValueBefore(DbgRecord *DPV,
InstListType::iterator Where) {
// We should never directly insert at the end of the block, new DPValues
// shouldn't be generated at times when there's no terminator.
assert(Where != end());
assert(Where->getParent() == this);
if (!Where->DbgMarker)
createMarker(Where);
assert(Where == end() || Where->getParent() == this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the first condition in this assert meant to be inverted? I would have thought that if Where == end(), then Where->getParent() == this would be trivially true!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If Where == end() then we can't dereference Where to check the parent matches. It's checking either Where is end() or, if it's not then the parent should match this bb.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so this is changing the condition here to account for insertions at end() (as for the OpenMP test above), while also preventing the error you just described. Got it!

bool InsertAtHead = Where.getHeadBit();
createMarker(&*Where);
Where->DbgMarker->insertDPValue(DPV, InsertAtHead);
DPMarker *M = createMarker(Where);
M->insertDPValue(DPV, InsertAtHead);
}

DPMarker *BasicBlock::getNextMarker(Instruction *I) {
Expand Down
132 changes: 93 additions & 39 deletions llvm/lib/IR/DIBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,35 +925,47 @@ DILexicalBlock *DIBuilder::createLexicalBlock(DIScope *Scope, DIFile *File,
File, Line, Col);
}

Instruction *DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
Instruction *InsertBefore) {
DbgInstPtr DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
Instruction *InsertBefore) {
return insertDeclare(Storage, VarInfo, Expr, DL, InsertBefore->getParent(),
InsertBefore);
}

Instruction *DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertAtEnd) {
DbgInstPtr DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertAtEnd) {
// If this block already has a terminator then insert this intrinsic before
// the terminator. Otherwise, put it at the end of the block.
Instruction *InsertBefore = InsertAtEnd->getTerminator();
return insertDeclare(Storage, VarInfo, Expr, DL, InsertAtEnd, InsertBefore);
}

DbgAssignIntrinsic *
DIBuilder::insertDbgAssign(Instruction *LinkedInstr, Value *Val,
DILocalVariable *SrcVar, DIExpression *ValExpr,
Value *Addr, DIExpression *AddrExpr,
const DILocation *DL) {
DbgInstPtr DIBuilder::insertDbgAssign(Instruction *LinkedInstr, Value *Val,
DILocalVariable *SrcVar,
DIExpression *ValExpr, Value *Addr,
DIExpression *AddrExpr,
const DILocation *DL) {
auto *Link = cast_or_null<DIAssignID>(
LinkedInstr->getMetadata(LLVMContext::MD_DIAssignID));
assert(Link && "Linked instruction must have DIAssign metadata attached");

if (M.IsNewDbgInfoFormat) {
DPValue *DPV = DPValue::createDPVAssign(Val, SrcVar, ValExpr, Link, Addr,
AddrExpr, DL);
BasicBlock *InsertBB = LinkedInstr->getParent();
// Insert after LinkedInstr.
BasicBlock::iterator NextIt = std::next(LinkedInstr->getIterator());
Instruction *InsertBefore = NextIt == InsertBB->end() ? nullptr : &*NextIt;
insertDPValue(DPV, InsertBB, InsertBefore, true);
return DPV;
}

LLVMContext &Ctx = LinkedInstr->getContext();
Module *M = LinkedInstr->getModule();
if (!AssignFn)
AssignFn = Intrinsic::getDeclaration(M, Intrinsic::dbg_assign);

auto *Link = LinkedInstr->getMetadata(LLVMContext::MD_DIAssignID);
assert(Link && "Linked instruction must have DIAssign metadata attached");

std::array<Value *, 6> Args = {
MetadataAsValue::get(Ctx, ValueAsMetadata::get(Val)),
MetadataAsValue::get(Ctx, SrcVar),
Expand All @@ -971,35 +983,36 @@ DIBuilder::insertDbgAssign(Instruction *LinkedInstr, Value *Val,
return DVI;
}

Instruction *DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL,
Instruction *InsertBefore) {
DbgInstPtr DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL,
Instruction *InsertBefore) {
return insertLabel(LabelInfo, DL,
InsertBefore ? InsertBefore->getParent() : nullptr,
InsertBefore);
}

Instruction *DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertAtEnd) {
DbgInstPtr DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertAtEnd) {
return insertLabel(LabelInfo, DL, InsertAtEnd, nullptr);
}

Instruction *DIBuilder::insertDbgValueIntrinsic(Value *V,
DILocalVariable *VarInfo,
DIExpression *Expr,
const DILocation *DL,
Instruction *InsertBefore) {
Instruction *DVI = insertDbgValueIntrinsic(
DbgInstPtr DIBuilder::insertDbgValueIntrinsic(Value *V,
DILocalVariable *VarInfo,
DIExpression *Expr,
const DILocation *DL,
Instruction *InsertBefore) {
DbgInstPtr DVI = insertDbgValueIntrinsic(
V, VarInfo, Expr, DL, InsertBefore ? InsertBefore->getParent() : nullptr,
InsertBefore);
cast<CallInst>(DVI)->setTailCall();
if (DVI.is<Instruction *>())
cast<CallInst>(DVI.get<Instruction *>())->setTailCall();
return DVI;
}

Instruction *DIBuilder::insertDbgValueIntrinsic(Value *V,
DILocalVariable *VarInfo,
DIExpression *Expr,
const DILocation *DL,
BasicBlock *InsertAtEnd) {
DbgInstPtr DIBuilder::insertDbgValueIntrinsic(Value *V,
DILocalVariable *VarInfo,
DIExpression *Expr,
const DILocation *DL,
BasicBlock *InsertAtEnd) {
return insertDbgValueIntrinsic(V, VarInfo, Expr, DL, InsertAtEnd, nullptr);
}

Expand All @@ -1023,24 +1036,37 @@ static Function *getDeclareIntrin(Module &M) {
return Intrinsic::getDeclaration(&M, Intrinsic::dbg_declare);
}

Instruction *DIBuilder::insertDbgValueIntrinsic(
DbgInstPtr DIBuilder::insertDbgValueIntrinsic(
llvm::Value *Val, DILocalVariable *VarInfo, DIExpression *Expr,
const DILocation *DL, BasicBlock *InsertBB, Instruction *InsertBefore) {
if (M.IsNewDbgInfoFormat) {
DPValue *DPV = DPValue::createDPValue(Val, VarInfo, Expr, DL);
insertDPValue(DPV, InsertBB, InsertBefore);
return DPV;
}

if (!ValueFn)
ValueFn = Intrinsic::getDeclaration(&M, Intrinsic::dbg_value);
return insertDbgIntrinsic(ValueFn, Val, VarInfo, Expr, DL, InsertBB,
InsertBefore);
}

Instruction *DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertBB,
Instruction *InsertBefore) {
DbgInstPtr DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
DIExpression *Expr, const DILocation *DL,
BasicBlock *InsertBB,
Instruction *InsertBefore) {
assert(VarInfo && "empty or invalid DILocalVariable* passed to dbg.declare");
assert(DL && "Expected debug loc");
assert(DL->getScope()->getSubprogram() ==
VarInfo->getScope()->getSubprogram() &&
"Expected matching subprograms");

if (M.IsNewDbgInfoFormat) {
DPValue *DPV = DPValue::createDPVDeclare(Storage, VarInfo, Expr, DL);
insertDPValue(DPV, InsertBB, InsertBefore);
return DPV;
}

if (!DeclareFn)
DeclareFn = getDeclareIntrin(M);

Expand All @@ -1055,6 +1081,23 @@ Instruction *DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo,
return B.CreateCall(DeclareFn, Args);
}

void DIBuilder::insertDPValue(DPValue *DPV, BasicBlock *InsertBB,
Instruction *InsertBefore, bool InsertAtHead) {
assert(InsertBefore || InsertBB);
trackIfUnresolved(DPV->getVariable());
trackIfUnresolved(DPV->getExpression());
if (DPV->isDbgAssign())
trackIfUnresolved(DPV->getAddressExpression());

BasicBlock::iterator InsertPt;
if (InsertBB && InsertBefore)
InsertPt = InsertBefore->getIterator();
else if (InsertBB)
InsertPt = InsertBB->end();
InsertPt.setHeadBit(InsertAtHead);
InsertBB->insertDPValueBefore(DPV, InsertPt);
}

Instruction *DIBuilder::insertDbgIntrinsic(llvm::Function *IntrinsicFn,
Value *V, DILocalVariable *VarInfo,
DIExpression *Expr,
Expand All @@ -1081,18 +1124,29 @@ Instruction *DIBuilder::insertDbgIntrinsic(llvm::Function *IntrinsicFn,
return B.CreateCall(IntrinsicFn, Args);
}

Instruction *DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertBB,
Instruction *InsertBefore) {
DbgInstPtr DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL,
BasicBlock *InsertBB,
Instruction *InsertBefore) {
assert(LabelInfo && "empty or invalid DILabel* passed to dbg.label");
assert(DL && "Expected debug loc");
assert(DL->getScope()->getSubprogram() ==
LabelInfo->getScope()->getSubprogram() &&
"Expected matching subprograms");

trackIfUnresolved(LabelInfo);
if (M.IsNewDbgInfoFormat) {
DPLabel *DPL = new DPLabel(LabelInfo, DL);
if (InsertBB && InsertBefore)
InsertBB->insertDPValueBefore(DPL, InsertBefore->getIterator());
else if (InsertBB)
InsertBB->insertDPValueBefore(DPL, InsertBB->end());
// FIXME: Use smart pointers for DbgRecord ownership management.
Copy link
Contributor

Choose a reason for hiding this comment

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

What would we need smart pointers for here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah that was a leftover comment to myself. Not needed here, was just musing on whether introducing smart pointers to type-ify DbgRecod ownership might be beneficial in the future. Deleted the comment!

return DPL;
}

if (!LabelFn)
LabelFn = Intrinsic::getDeclaration(&M, Intrinsic::dbg_label);

trackIfUnresolved(LabelInfo);
Value *Args[] = {MetadataAsValue::get(VMContext, LabelInfo)};

IRBuilder<> B(DL->getContext());
Expand Down
Loading