[RemoveDIs] Update Coroutine passes to handle DPValues #74480
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
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-coroutines Author: Orlando Cazalet-Hyams (OCHyams) ChangesAs part of the RemoveDIs project, transitioning to non-instruction debug info, all debug intrinsic handling code needs to be duplicated to handle DPValues. --try-experimental-debuginfo-iterators enables the new debug mode in tests if the CMake option has been enabled.
Depends on #73500, #74090, #74091. Patch is 26.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/74480.diff 13 Files Affected:
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 3ac5fafd887df..c0de37facd43f 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -1080,6 +1080,8 @@ void BasicBlock::insertDPValueBefore(DPValue *DPV,
// shouldn't be generated at times when there's no terminator.
assert(Where != end());
assert(Where->getParent() == this);
+ if (!Where->DbgMarker)
+ createMarker(Where);
bool InsertAtHead = Where.getHeadBit();
Where->DbgMarker->insertDPValue(DPV, InsertAtHead);
}
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 1134b20880f18..4b73093c8aa0b 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -963,12 +963,18 @@ static void cacheDIVar(FrameDataInfo &FrameData,
if (DIVarCache.contains(V))
continue;
- auto DDIs = FindDbgDeclareUses(V);
- auto *I = llvm::find_if(DDIs, [](DbgDeclareInst *DDI) {
- return DDI->getExpression()->getNumElements() == 0;
- });
- if (I != DDIs.end())
- DIVarCache.insert({V, (*I)->getVariable()});
+ SmallVector<DbgDeclareInst *> DDIs;
+ SmallVector<DPValue *> DPVs;
+ findDbgDeclares(DDIs, V, &DPVs);
+ auto CacheIt = [&DIVarCache, V](auto &Container) {
+ auto *I = llvm::find_if(Container, [](auto *DDI) {
+ return DDI->getExpression()->getNumElements() == 0;
+ });
+ if (I != Container.end())
+ DIVarCache.insert({V, (*I)->getVariable()});
+ };
+ CacheIt(DDIs);
+ CacheIt(DPVs);
}
}
@@ -1119,15 +1125,26 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
assert(PromiseAlloca &&
"Coroutine with switch ABI should own Promise alloca");
- TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(PromiseAlloca);
- if (DIs.empty())
+ SmallVector<DbgDeclareInst *> DIs;
+ SmallVector<DPValue *> DPVs;
+ findDbgDeclares(DIs, PromiseAlloca, &DPVs);
+
+ DILocalVariable *PromiseDIVariable = nullptr;
+ DILocation *DILoc = nullptr;
+ if (!DIs.empty()) {
+ DbgDeclareInst *PromiseDDI = DIs.front();
+ PromiseDIVariable = PromiseDDI->getVariable();
+ DILoc = PromiseDDI->getDebugLoc().get();
+ } else if (!DPVs.empty()) {
+ DPValue *PromiseDPV = DPVs.front();
+ PromiseDIVariable = PromiseDPV->getVariable();
+ DILoc = PromiseDPV->getDebugLoc().get();
+ } else {
return;
+ }
- DbgDeclareInst *PromiseDDI = DIs.front();
- DILocalVariable *PromiseDIVariable = PromiseDDI->getVariable();
DILocalScope *PromiseDIScope = PromiseDIVariable->getScope();
DIFile *DFile = PromiseDIScope->getFile();
- DILocation *DILoc = PromiseDDI->getDebugLoc().get();
unsigned LineNum = PromiseDIVariable->getLine();
DICompositeType *FrameDITy = DBuilder.createStructType(
@@ -1241,7 +1258,7 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
auto *FrameDIVar = DBuilder.createAutoVariable(PromiseDIScope, "__coro_frame",
DFile, LineNum, FrameDITy,
true, DINode::FlagArtificial);
- assert(FrameDIVar->isValidLocationForIntrinsic(PromiseDDI->getDebugLoc()));
+ assert(FrameDIVar->isValidLocationForIntrinsic(DILoc));
// Subprogram would have ContainedNodes field which records the debug
// variables it contained. So we need to add __coro_frame to the
@@ -1259,9 +1276,17 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
7, (MDTuple::get(F.getContext(), RetainedNodesVec)));
}
- DBuilder.insertDeclare(Shape.FramePtr, FrameDIVar,
- DBuilder.createExpression(), DILoc,
- Shape.getInsertPtAfterFramePtr());
+ if (UseNewDbgInfoFormat) {
+ DPValue *NewDPV = new DPValue(ValueAsMetadata::get(Shape.FramePtr),
+ FrameDIVar, DBuilder.createExpression(),
+ DILoc, DPValue::LocationType::Declare);
+ BasicBlock::iterator It = Shape.getInsertPtAfterFramePtr();
+ It->getParent()->insertDPValueBefore(NewDPV, It);
+ } else {
+ DBuilder.insertDeclare(Shape.FramePtr, FrameDIVar,
+ DBuilder.createExpression(), DILoc,
+ &*Shape.getInsertPtAfterFramePtr());
+ }
}
// Build a struct that will keep state for an active coroutine.
@@ -1769,7 +1794,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
if (auto *Arg = dyn_cast<Argument>(Def)) {
// For arguments, we will place the store instruction right after
// the coroutine frame pointer instruction, i.e. coro.begin.
- InsertPt = Shape.getInsertPtAfterFramePtr()->getIterator();
+ InsertPt = Shape.getInsertPtAfterFramePtr();
// If we're spilling an Argument, make sure we clear 'nocapture'
// from the coroutine function.
@@ -1786,7 +1811,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
if (!DT.dominates(CB, I)) {
// If it is not dominated by CoroBegin, then spill should be
// inserted immediately after CoroFrame is computed.
- InsertPt = Shape.getInsertPtAfterFramePtr()->getIterator();
+ InsertPt = Shape.getInsertPtAfterFramePtr();
} else if (auto *II = dyn_cast<InvokeInst>(I)) {
// If we are spilling the result of the invoke instruction, split
// the normal edge and insert the spill in the new block.
@@ -1840,7 +1865,9 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
FrameTy->getElementType(FrameData.getFieldIndex(E.first)), GEP,
SpillAlignment, E.first->getName() + Twine(".reload"));
- TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(Def);
+ SmallVector<DbgDeclareInst *> DIs;
+ SmallVector<DPValue *> DPVs;
+ findDbgDeclares(DIs, Def, &DPVs);
// Try best to find dbg.declare. If the spill is a temp, there may not
// be a direct dbg.declare. Walk up the load chain to find one from an
// alias.
@@ -1854,24 +1881,37 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
CurDef = LdInst->getPointerOperand();
if (!isa<AllocaInst, LoadInst>(CurDef))
break;
- DIs = FindDbgDeclareUses(CurDef);
+ DIs.clear();
+ DPVs.clear();
+ findDbgDeclares(DIs, CurDef, &DPVs);
}
}
- for (DbgDeclareInst *DDI : DIs) {
+ auto SalvageOne = [&](auto *DDI) {
bool AllowUnresolved = false;
// This dbg.declare is preserved for all coro-split function
// fragments. It will be unreachable in the main function, and
// processed by coro::salvageDebugInfo() by CoroCloner.
- DIBuilder(*CurrentBlock->getParent()->getParent(), AllowUnresolved)
- .insertDeclare(CurrentReload, DDI->getVariable(),
- DDI->getExpression(), DDI->getDebugLoc(),
- &*Builder.GetInsertPoint());
+ if (UseNewDbgInfoFormat) {
+ DPValue *NewDPV =
+ new DPValue(ValueAsMetadata::get(CurrentReload),
+ DDI->getVariable(), DDI->getExpression(),
+ DDI->getDebugLoc(), DPValue::LocationType::Declare);
+ Builder.GetInsertPoint()->getParent()->insertDPValueBefore(
+ NewDPV, Builder.GetInsertPoint());
+ } else {
+ DIBuilder(*CurrentBlock->getParent()->getParent(), AllowUnresolved)
+ .insertDeclare(CurrentReload, DDI->getVariable(),
+ DDI->getExpression(), DDI->getDebugLoc(),
+ &*Builder.GetInsertPoint());
+ }
// This dbg.declare is for the main function entry point. It
// will be deleted in all coro-split functions.
coro::salvageDebugInfo(ArgToAllocaMap, DDI, Shape.OptimizeFrame,
false /*UseEntryValue*/);
- }
+ };
+ for_each(DIs, SalvageOne);
+ for_each(DPVs, SalvageOne);
}
// If we have a single edge PHINode, remove it and replace it with a
@@ -1889,6 +1929,10 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
// Replace all uses of CurrentValue in the current instruction with
// reload.
U->replaceUsesOfWith(Def, CurrentReload);
+ // Instructions are added to Def's user list if the attached
+ // debug records use Def. Update those now.
+ for (auto &DPV: U->getDbgValueRange())
+ DPV.replaceVariableLocationOp(Def, CurrentReload, true);
}
}
@@ -1939,9 +1983,12 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
G->setName(Alloca->getName() + Twine(".reload.addr"));
SmallVector<DbgVariableIntrinsic *, 4> DIs;
- findDbgUsers(DIs, Alloca);
+ SmallVector<DPValue *> DPValues;
+ findDbgUsers(DIs, Alloca, &DPValues);
for (auto *DVI : DIs)
DVI->replaceUsesOfWith(Alloca, G);
+ for (auto *DPV : DPValues)
+ DPV->replaceVariableLocationOp(Alloca, G);
for (Instruction *I : UsersToUpdate) {
// It is meaningless to retain the lifetime intrinsics refer for the
@@ -1955,7 +2002,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
I->replaceUsesOfWith(Alloca, G);
}
}
- Builder.SetInsertPoint(Shape.getInsertPtAfterFramePtr());
+ Builder.SetInsertPoint(&*Shape.getInsertPtAfterFramePtr());
for (const auto &A : FrameData.Allocas) {
AllocaInst *Alloca = A.Alloca;
if (A.MayWriteBeforeCoroBegin) {
@@ -2016,7 +2063,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
isa<BitCastInst>(Inst);
});
if (HasAccessingPromiseBeforeCB) {
- Builder.SetInsertPoint(Shape.getInsertPtAfterFramePtr());
+ Builder.SetInsertPoint(&*Shape.getInsertPtAfterFramePtr());
auto *G = GetFramePointer(PA);
auto *Value = Builder.CreateLoad(PA->getAllocatedType(), PA);
Builder.CreateStore(Value, G);
@@ -2798,21 +2845,16 @@ static void collectFrameAlloca(AllocaInst *AI, coro::Shape &Shape,
Visitor.getMayWriteBeforeCoroBegin());
}
-void coro::salvageDebugInfo(
- SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
- DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool UseEntryValue) {
- Function *F = DVI->getFunction();
+static std::optional<std::pair<Value *, DIExpression *>>
+salvageDebugInfoImpl(SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
+ bool OptimizeFrame, bool UseEntryValue, Function *F,
+ Value *Storage, DIExpression *Expr,
+ bool SkipOutermostLoad) {
IRBuilder<> Builder(F->getContext());
auto InsertPt = F->getEntryBlock().getFirstInsertionPt();
while (isa<IntrinsicInst>(InsertPt))
++InsertPt;
Builder.SetInsertPoint(&F->getEntryBlock(), InsertPt);
- DIExpression *Expr = DVI->getExpression();
- // Follow the pointer arithmetic all the way to the incoming
- // function argument and convert into a DIExpression.
- bool SkipOutermostLoad = !isa<DbgValueInst>(DVI);
- Value *Storage = DVI->getVariableLocationOp(0);
- Value *OriginalStorage = Storage;
while (auto *Inst = dyn_cast_or_null<Instruction>(Storage)) {
if (auto *LdInst = dyn_cast<LoadInst>(Inst)) {
@@ -2844,7 +2886,7 @@ void coro::salvageDebugInfo(
SkipOutermostLoad = false;
}
if (!Storage)
- return;
+ return std::nullopt;
auto *StorageAsArg = dyn_cast<Argument>(Storage);
const bool IsSwiftAsyncArg =
@@ -2880,6 +2922,28 @@ void coro::salvageDebugInfo(
Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
}
+ return {{Storage, Expr}};
+}
+
+void coro::salvageDebugInfo(
+ SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
+ DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool UseEntryValue) {
+
+ Function *F = DVI->getFunction();
+ // Follow the pointer arithmetic all the way to the incoming
+ // function argument and convert into a DIExpression.
+ bool SkipOutermostLoad = !isa<DbgValueInst>(DVI);
+ Value *OriginalStorage = DVI->getVariableLocationOp(0);
+
+ auto SalvagedInfo = ::salvageDebugInfoImpl(
+ ArgToAllocaMap, OptimizeFrame, UseEntryValue, F, OriginalStorage,
+ DVI->getExpression(), SkipOutermostLoad);
+ if (!SalvagedInfo)
+ return;
+
+ Value *Storage = SalvagedInfo->first;
+ DIExpression *Expr = SalvagedInfo->second;
+
DVI->replaceVariableLocationOp(OriginalStorage, Storage);
DVI->setExpression(Expr);
// We only hoist dbg.declare today since it doesn't make sense to hoist
@@ -2896,6 +2960,43 @@ void coro::salvageDebugInfo(
}
}
+void coro::salvageDebugInfo(
+ SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, DPValue *DPV,
+ bool OptimizeFrame, bool UseEntryValue) {
+
+ Function *F = DPV->getFunction();
+ // Follow the pointer arithmetic all the way to the incoming
+ // function argument and convert into a DIExpression.
+ bool SkipOutermostLoad = DPV->getType() == DPValue::LocationType::Declare;
+ Value *OriginalStorage = DPV->getVariableLocationOp(0);
+
+ auto SalvagedInfo = ::salvageDebugInfoImpl(
+ ArgToAllocaMap, OptimizeFrame, UseEntryValue, F, OriginalStorage,
+ DPV->getExpression(), SkipOutermostLoad);
+ if (!SalvagedInfo)
+ return;
+
+ Value *Storage = SalvagedInfo->first;
+ DIExpression *Expr = SalvagedInfo->second;
+
+ DPV->replaceVariableLocationOp(OriginalStorage, Storage);
+ DPV->setExpression(Expr);
+ // We only hoist dbg.declare today since it doesn't make sense to hoist
+ // dbg.value since it does not have the same function wide guarantees that
+ // dbg.declare does.
+ if (DPV->getType() == DPValue::LocationType::Declare) {
+ std::optional<BasicBlock::iterator> InsertPt;
+ if (auto *I = dyn_cast<Instruction>(Storage))
+ InsertPt = I->getInsertionPointAfterDef();
+ else if (isa<Argument>(Storage))
+ InsertPt = F->getEntryBlock().begin();
+ if (InsertPt) {
+ DPV->removeFromParent();
+ (*InsertPt)->getParent()->insertDPValueBefore(DPV, *InsertPt);
+ }
+ }
+}
+
static void doRematerializations(
Function &F, SuspendCrossingInfo &Checker,
const std::function<bool(Instruction &)> &MaterializableCallback) {
@@ -3083,10 +3184,15 @@ void coro::buildCoroutineFrame(
for (auto &Iter : FrameData.Spills) {
auto *V = Iter.first;
SmallVector<DbgValueInst *, 16> DVIs;
- findDbgValues(DVIs, V);
+ SmallVector<DPValue *, 16> DPVs;
+ findDbgValues(DVIs, V, &DPVs);
for (DbgValueInst *DVI : DVIs)
if (Checker.isDefinitionAcrossSuspend(*V, DVI))
FrameData.Spills[V].push_back(DVI);
+ // Add the instructions which carry debug info that is in the frame.
+ for (DPValue *DPV : DPVs)
+ if (Checker.isDefinitionAcrossSuspend(*V, DPV->Marker->MarkedInstr))
+ FrameData.Spills[V].push_back(DPV->Marker->MarkedInstr);
}
LLVM_DEBUG(dumpSpills("Spills", FrameData.Spills));
diff --git a/llvm/lib/Transforms/Coroutines/CoroInternal.h b/llvm/lib/Transforms/Coroutines/CoroInternal.h
index 0856c4925cc5d..1022f7a2979f5 100644
--- a/llvm/lib/Transforms/Coroutines/CoroInternal.h
+++ b/llvm/lib/Transforms/Coroutines/CoroInternal.h
@@ -33,6 +33,10 @@ void replaceCoroFree(CoroIdInst *CoroId, bool Elide);
void salvageDebugInfo(
SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool IsEntryPoint);
+/// See overload.
+void salvageDebugInfo(
+ SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, DPValue *DPV,
+ bool OptimizeFrame, bool UseEntryValue);
// Keeps data and helper functions for lowering coroutine intrinsics.
struct LowererBase {
@@ -240,10 +244,13 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
return nullptr;
}
- Instruction *getInsertPtAfterFramePtr() const {
- if (auto *I = dyn_cast<Instruction>(FramePtr))
- return I->getNextNode();
- return &cast<Argument>(FramePtr)->getParent()->getEntryBlock().front();
+ BasicBlock::iterator getInsertPtAfterFramePtr() const {
+ if (auto *I = dyn_cast<Instruction>(FramePtr)) {
+ BasicBlock::iterator It = std::next(I->getIterator());
+ It.setHeadBit(true); // Copy pre-RemoveDIs behaviour.
+ return It;
+ }
+ return cast<Argument>(FramePtr)->getParent()->getEntryBlock().begin();
}
/// Allocate memory according to the rules of the active lowering.
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 244580f503d5b..504aa912ac266 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -726,11 +726,14 @@ static void replaceSwiftErrorOps(Function &F, coro::Shape &Shape,
/// Returns all DbgVariableIntrinsic in F.
static SmallVector<DbgVariableIntrinsic *, 8>
-collectDbgVariableIntrinsics(Function &F) {
+collectDbgVariableIntrinsics(Function &F, SmallVector<DPValue *> &DPValues) {
SmallVector<DbgVariableIntrinsic *, 8> Intrinsics;
- for (auto &I : instructions(F))
+ for (auto &I : instructions(F)) {
+ for (DPValue &DPV : I.getDbgValueRange())
+ DPValues.push_back(&DPV);
if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I))
Intrinsics.push_back(DVI);
+ }
return Intrinsics;
}
@@ -739,8 +742,9 @@ void CoroCloner::replaceSwiftErrorOps() {
}
void CoroCloner::salvageDebugInfo() {
+ SmallVector<DPValue *> DPValues;
SmallVector<DbgVariableIntrinsic *, 8> Worklist =
- collectDbgVariableIntrinsics(*NewF);
+ collectDbgVariableIntrinsics(*NewF, DPValues);
SmallDenseMap<Argument *, AllocaInst *, 4> ArgToAllocaMap;
// Only 64-bit ABIs have a register we can refer to with the entry value.
@@ -749,6 +753,9 @@ void CoroCloner::salvageDebugInfo() {
for (DbgVariableIntrinsic *DVI : Worklist)
coro::salvageDebugInfo(ArgToAllocaMap, DVI, Shape.OptimizeFrame,
UseEntryValue);
+ for (DPValue *DPV : DPValues)
+ coro::salvageDebugInfo(ArgToAllocaMap, DPV, Shape.OptimizeFrame,
+ UseEntryValue);
// Remove all salvaged dbg.declare intrinsics that became
// either unreachable or stale due to the CoroSplit transformation.
@@ -757,7 +764,7 @@ void CoroCloner::salvageDebugInfo() {
return !isPotentiallyReachable(&NewF->getEntryBlock(), BB, nullptr,
&DomTree);
};
- for (DbgVariableIntrinsic *DVI : Worklist) {
+ auto RemoveOne = [&](auto *DVI) {
if (IsUnreachableBlock(DVI->getParent()))
DVI->eraseFromParent();
else if (isa_and_nonnull<AllocaInst>(DVI->getVariableLocationOp(0))) {
@@ -770,7 +777,9 @@ void CoroCloner::salvageDebugInfo() {
if (!Uses)
DVI->eraseFromParent();
}
- }
+ };
+ for_each(Worklist, RemoveOne);
+ for_each(DPValues, RemoveOne);
}
void CoroCloner::replaceEntryBlock() {
@@ -1243,7 +1252,7 @@ static void updateCoroFrame(coro::Shape &Shape, Function *ResumeFn,
Function *DestroyFn, Function *CleanupFn) {
assert(Shape.ABI == coro::ABI::Switch);
- IRBuilder<> Builder(Shape.getInsertPtAfterFramePtr());
+ IRBuilder<> Builder(&*Shape.getInsertPtAfterFramePtr());
auto *ResumeAddr = Builder.CreateStructGEP(
Shape.FrameTy, Shape.FramePtr, coro::Shape::SwitchFieldIndex::Resume,
@@ -2039,10 +2048,13 @@ splitCoroutine(Function &F, SmallVectorImpl<Function *> &Clones,
// original function. The Cloner has already salvaged debug info in the new
// coroutine funclets.
SmallDenseMap<Argument *, AllocaInst *, 4> ArgToAllocaMap;
- for (auto *DDI : collectDbgVariableIntrinsics(F))
+ SmallVector<DPValue *> DPValues;
+ for (auto *DDI : collectDbgVariableIntrinsics(F, DPValues))
coro::salvageDebugInfo(ArgToAllocaMap, DDI, Shape.OptimizeFrame,
false /*UseEntryValue*/);
-
+ for (DPValue *DPV : DPValues)
+ coro::salvageDebugInfo(ArgToAllocaMap, DPV, Shape.OptimizeFrame,
+ false /*UseEntryValue*/);
return Shape;
}
diff --git a/llvm/test/Transforms/Coroutin...
[truncated]
|
|
@llvm/pr-subscribers-llvm-transforms Author: Orlando Cazalet-Hyams (OCHyams) ChangesAs part of the RemoveDIs project, transitioning to non-instruction debug info, all debug intrinsic handling code needs to be duplicated to handle DPValues. --try-experimental-debuginfo-iterators enables the new debug mode in tests if the CMake option has been enabled.
Depends on #73500, #74090, #74091. Patch is 26.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/74480.diff 13 Files Affected:
diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp
index 3ac5fafd887df..c0de37facd43f 100644
--- a/llvm/lib/IR/BasicBlock.cpp
+++ b/llvm/lib/IR/BasicBlock.cpp
@@ -1080,6 +1080,8 @@ void BasicBlock::insertDPValueBefore(DPValue *DPV,
// shouldn't be generated at times when there's no terminator.
assert(Where != end());
assert(Where->getParent() == this);
+ if (!Where->DbgMarker)
+ createMarker(Where);
bool InsertAtHead = Where.getHeadBit();
Where->DbgMarker->insertDPValue(DPV, InsertAtHead);
}
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 1134b20880f18..4b73093c8aa0b 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -963,12 +963,18 @@ static void cacheDIVar(FrameDataInfo &FrameData,
if (DIVarCache.contains(V))
continue;
- auto DDIs = FindDbgDeclareUses(V);
- auto *I = llvm::find_if(DDIs, [](DbgDeclareInst *DDI) {
- return DDI->getExpression()->getNumElements() == 0;
- });
- if (I != DDIs.end())
- DIVarCache.insert({V, (*I)->getVariable()});
+ SmallVector<DbgDeclareInst *> DDIs;
+ SmallVector<DPValue *> DPVs;
+ findDbgDeclares(DDIs, V, &DPVs);
+ auto CacheIt = [&DIVarCache, V](auto &Container) {
+ auto *I = llvm::find_if(Container, [](auto *DDI) {
+ return DDI->getExpression()->getNumElements() == 0;
+ });
+ if (I != Container.end())
+ DIVarCache.insert({V, (*I)->getVariable()});
+ };
+ CacheIt(DDIs);
+ CacheIt(DPVs);
}
}
@@ -1119,15 +1125,26 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
assert(PromiseAlloca &&
"Coroutine with switch ABI should own Promise alloca");
- TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(PromiseAlloca);
- if (DIs.empty())
+ SmallVector<DbgDeclareInst *> DIs;
+ SmallVector<DPValue *> DPVs;
+ findDbgDeclares(DIs, PromiseAlloca, &DPVs);
+
+ DILocalVariable *PromiseDIVariable = nullptr;
+ DILocation *DILoc = nullptr;
+ if (!DIs.empty()) {
+ DbgDeclareInst *PromiseDDI = DIs.front();
+ PromiseDIVariable = PromiseDDI->getVariable();
+ DILoc = PromiseDDI->getDebugLoc().get();
+ } else if (!DPVs.empty()) {
+ DPValue *PromiseDPV = DPVs.front();
+ PromiseDIVariable = PromiseDPV->getVariable();
+ DILoc = PromiseDPV->getDebugLoc().get();
+ } else {
return;
+ }
- DbgDeclareInst *PromiseDDI = DIs.front();
- DILocalVariable *PromiseDIVariable = PromiseDDI->getVariable();
DILocalScope *PromiseDIScope = PromiseDIVariable->getScope();
DIFile *DFile = PromiseDIScope->getFile();
- DILocation *DILoc = PromiseDDI->getDebugLoc().get();
unsigned LineNum = PromiseDIVariable->getLine();
DICompositeType *FrameDITy = DBuilder.createStructType(
@@ -1241,7 +1258,7 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
auto *FrameDIVar = DBuilder.createAutoVariable(PromiseDIScope, "__coro_frame",
DFile, LineNum, FrameDITy,
true, DINode::FlagArtificial);
- assert(FrameDIVar->isValidLocationForIntrinsic(PromiseDDI->getDebugLoc()));
+ assert(FrameDIVar->isValidLocationForIntrinsic(DILoc));
// Subprogram would have ContainedNodes field which records the debug
// variables it contained. So we need to add __coro_frame to the
@@ -1259,9 +1276,17 @@ static void buildFrameDebugInfo(Function &F, coro::Shape &Shape,
7, (MDTuple::get(F.getContext(), RetainedNodesVec)));
}
- DBuilder.insertDeclare(Shape.FramePtr, FrameDIVar,
- DBuilder.createExpression(), DILoc,
- Shape.getInsertPtAfterFramePtr());
+ if (UseNewDbgInfoFormat) {
+ DPValue *NewDPV = new DPValue(ValueAsMetadata::get(Shape.FramePtr),
+ FrameDIVar, DBuilder.createExpression(),
+ DILoc, DPValue::LocationType::Declare);
+ BasicBlock::iterator It = Shape.getInsertPtAfterFramePtr();
+ It->getParent()->insertDPValueBefore(NewDPV, It);
+ } else {
+ DBuilder.insertDeclare(Shape.FramePtr, FrameDIVar,
+ DBuilder.createExpression(), DILoc,
+ &*Shape.getInsertPtAfterFramePtr());
+ }
}
// Build a struct that will keep state for an active coroutine.
@@ -1769,7 +1794,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
if (auto *Arg = dyn_cast<Argument>(Def)) {
// For arguments, we will place the store instruction right after
// the coroutine frame pointer instruction, i.e. coro.begin.
- InsertPt = Shape.getInsertPtAfterFramePtr()->getIterator();
+ InsertPt = Shape.getInsertPtAfterFramePtr();
// If we're spilling an Argument, make sure we clear 'nocapture'
// from the coroutine function.
@@ -1786,7 +1811,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
if (!DT.dominates(CB, I)) {
// If it is not dominated by CoroBegin, then spill should be
// inserted immediately after CoroFrame is computed.
- InsertPt = Shape.getInsertPtAfterFramePtr()->getIterator();
+ InsertPt = Shape.getInsertPtAfterFramePtr();
} else if (auto *II = dyn_cast<InvokeInst>(I)) {
// If we are spilling the result of the invoke instruction, split
// the normal edge and insert the spill in the new block.
@@ -1840,7 +1865,9 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
FrameTy->getElementType(FrameData.getFieldIndex(E.first)), GEP,
SpillAlignment, E.first->getName() + Twine(".reload"));
- TinyPtrVector<DbgDeclareInst *> DIs = FindDbgDeclareUses(Def);
+ SmallVector<DbgDeclareInst *> DIs;
+ SmallVector<DPValue *> DPVs;
+ findDbgDeclares(DIs, Def, &DPVs);
// Try best to find dbg.declare. If the spill is a temp, there may not
// be a direct dbg.declare. Walk up the load chain to find one from an
// alias.
@@ -1854,24 +1881,37 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
CurDef = LdInst->getPointerOperand();
if (!isa<AllocaInst, LoadInst>(CurDef))
break;
- DIs = FindDbgDeclareUses(CurDef);
+ DIs.clear();
+ DPVs.clear();
+ findDbgDeclares(DIs, CurDef, &DPVs);
}
}
- for (DbgDeclareInst *DDI : DIs) {
+ auto SalvageOne = [&](auto *DDI) {
bool AllowUnresolved = false;
// This dbg.declare is preserved for all coro-split function
// fragments. It will be unreachable in the main function, and
// processed by coro::salvageDebugInfo() by CoroCloner.
- DIBuilder(*CurrentBlock->getParent()->getParent(), AllowUnresolved)
- .insertDeclare(CurrentReload, DDI->getVariable(),
- DDI->getExpression(), DDI->getDebugLoc(),
- &*Builder.GetInsertPoint());
+ if (UseNewDbgInfoFormat) {
+ DPValue *NewDPV =
+ new DPValue(ValueAsMetadata::get(CurrentReload),
+ DDI->getVariable(), DDI->getExpression(),
+ DDI->getDebugLoc(), DPValue::LocationType::Declare);
+ Builder.GetInsertPoint()->getParent()->insertDPValueBefore(
+ NewDPV, Builder.GetInsertPoint());
+ } else {
+ DIBuilder(*CurrentBlock->getParent()->getParent(), AllowUnresolved)
+ .insertDeclare(CurrentReload, DDI->getVariable(),
+ DDI->getExpression(), DDI->getDebugLoc(),
+ &*Builder.GetInsertPoint());
+ }
// This dbg.declare is for the main function entry point. It
// will be deleted in all coro-split functions.
coro::salvageDebugInfo(ArgToAllocaMap, DDI, Shape.OptimizeFrame,
false /*UseEntryValue*/);
- }
+ };
+ for_each(DIs, SalvageOne);
+ for_each(DPVs, SalvageOne);
}
// If we have a single edge PHINode, remove it and replace it with a
@@ -1889,6 +1929,10 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
// Replace all uses of CurrentValue in the current instruction with
// reload.
U->replaceUsesOfWith(Def, CurrentReload);
+ // Instructions are added to Def's user list if the attached
+ // debug records use Def. Update those now.
+ for (auto &DPV: U->getDbgValueRange())
+ DPV.replaceVariableLocationOp(Def, CurrentReload, true);
}
}
@@ -1939,9 +1983,12 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
G->setName(Alloca->getName() + Twine(".reload.addr"));
SmallVector<DbgVariableIntrinsic *, 4> DIs;
- findDbgUsers(DIs, Alloca);
+ SmallVector<DPValue *> DPValues;
+ findDbgUsers(DIs, Alloca, &DPValues);
for (auto *DVI : DIs)
DVI->replaceUsesOfWith(Alloca, G);
+ for (auto *DPV : DPValues)
+ DPV->replaceVariableLocationOp(Alloca, G);
for (Instruction *I : UsersToUpdate) {
// It is meaningless to retain the lifetime intrinsics refer for the
@@ -1955,7 +2002,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
I->replaceUsesOfWith(Alloca, G);
}
}
- Builder.SetInsertPoint(Shape.getInsertPtAfterFramePtr());
+ Builder.SetInsertPoint(&*Shape.getInsertPtAfterFramePtr());
for (const auto &A : FrameData.Allocas) {
AllocaInst *Alloca = A.Alloca;
if (A.MayWriteBeforeCoroBegin) {
@@ -2016,7 +2063,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
isa<BitCastInst>(Inst);
});
if (HasAccessingPromiseBeforeCB) {
- Builder.SetInsertPoint(Shape.getInsertPtAfterFramePtr());
+ Builder.SetInsertPoint(&*Shape.getInsertPtAfterFramePtr());
auto *G = GetFramePointer(PA);
auto *Value = Builder.CreateLoad(PA->getAllocatedType(), PA);
Builder.CreateStore(Value, G);
@@ -2798,21 +2845,16 @@ static void collectFrameAlloca(AllocaInst *AI, coro::Shape &Shape,
Visitor.getMayWriteBeforeCoroBegin());
}
-void coro::salvageDebugInfo(
- SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
- DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool UseEntryValue) {
- Function *F = DVI->getFunction();
+static std::optional<std::pair<Value *, DIExpression *>>
+salvageDebugInfoImpl(SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
+ bool OptimizeFrame, bool UseEntryValue, Function *F,
+ Value *Storage, DIExpression *Expr,
+ bool SkipOutermostLoad) {
IRBuilder<> Builder(F->getContext());
auto InsertPt = F->getEntryBlock().getFirstInsertionPt();
while (isa<IntrinsicInst>(InsertPt))
++InsertPt;
Builder.SetInsertPoint(&F->getEntryBlock(), InsertPt);
- DIExpression *Expr = DVI->getExpression();
- // Follow the pointer arithmetic all the way to the incoming
- // function argument and convert into a DIExpression.
- bool SkipOutermostLoad = !isa<DbgValueInst>(DVI);
- Value *Storage = DVI->getVariableLocationOp(0);
- Value *OriginalStorage = Storage;
while (auto *Inst = dyn_cast_or_null<Instruction>(Storage)) {
if (auto *LdInst = dyn_cast<LoadInst>(Inst)) {
@@ -2844,7 +2886,7 @@ void coro::salvageDebugInfo(
SkipOutermostLoad = false;
}
if (!Storage)
- return;
+ return std::nullopt;
auto *StorageAsArg = dyn_cast<Argument>(Storage);
const bool IsSwiftAsyncArg =
@@ -2880,6 +2922,28 @@ void coro::salvageDebugInfo(
Expr = DIExpression::prepend(Expr, DIExpression::DerefBefore);
}
+ return {{Storage, Expr}};
+}
+
+void coro::salvageDebugInfo(
+ SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
+ DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool UseEntryValue) {
+
+ Function *F = DVI->getFunction();
+ // Follow the pointer arithmetic all the way to the incoming
+ // function argument and convert into a DIExpression.
+ bool SkipOutermostLoad = !isa<DbgValueInst>(DVI);
+ Value *OriginalStorage = DVI->getVariableLocationOp(0);
+
+ auto SalvagedInfo = ::salvageDebugInfoImpl(
+ ArgToAllocaMap, OptimizeFrame, UseEntryValue, F, OriginalStorage,
+ DVI->getExpression(), SkipOutermostLoad);
+ if (!SalvagedInfo)
+ return;
+
+ Value *Storage = SalvagedInfo->first;
+ DIExpression *Expr = SalvagedInfo->second;
+
DVI->replaceVariableLocationOp(OriginalStorage, Storage);
DVI->setExpression(Expr);
// We only hoist dbg.declare today since it doesn't make sense to hoist
@@ -2896,6 +2960,43 @@ void coro::salvageDebugInfo(
}
}
+void coro::salvageDebugInfo(
+ SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, DPValue *DPV,
+ bool OptimizeFrame, bool UseEntryValue) {
+
+ Function *F = DPV->getFunction();
+ // Follow the pointer arithmetic all the way to the incoming
+ // function argument and convert into a DIExpression.
+ bool SkipOutermostLoad = DPV->getType() == DPValue::LocationType::Declare;
+ Value *OriginalStorage = DPV->getVariableLocationOp(0);
+
+ auto SalvagedInfo = ::salvageDebugInfoImpl(
+ ArgToAllocaMap, OptimizeFrame, UseEntryValue, F, OriginalStorage,
+ DPV->getExpression(), SkipOutermostLoad);
+ if (!SalvagedInfo)
+ return;
+
+ Value *Storage = SalvagedInfo->first;
+ DIExpression *Expr = SalvagedInfo->second;
+
+ DPV->replaceVariableLocationOp(OriginalStorage, Storage);
+ DPV->setExpression(Expr);
+ // We only hoist dbg.declare today since it doesn't make sense to hoist
+ // dbg.value since it does not have the same function wide guarantees that
+ // dbg.declare does.
+ if (DPV->getType() == DPValue::LocationType::Declare) {
+ std::optional<BasicBlock::iterator> InsertPt;
+ if (auto *I = dyn_cast<Instruction>(Storage))
+ InsertPt = I->getInsertionPointAfterDef();
+ else if (isa<Argument>(Storage))
+ InsertPt = F->getEntryBlock().begin();
+ if (InsertPt) {
+ DPV->removeFromParent();
+ (*InsertPt)->getParent()->insertDPValueBefore(DPV, *InsertPt);
+ }
+ }
+}
+
static void doRematerializations(
Function &F, SuspendCrossingInfo &Checker,
const std::function<bool(Instruction &)> &MaterializableCallback) {
@@ -3083,10 +3184,15 @@ void coro::buildCoroutineFrame(
for (auto &Iter : FrameData.Spills) {
auto *V = Iter.first;
SmallVector<DbgValueInst *, 16> DVIs;
- findDbgValues(DVIs, V);
+ SmallVector<DPValue *, 16> DPVs;
+ findDbgValues(DVIs, V, &DPVs);
for (DbgValueInst *DVI : DVIs)
if (Checker.isDefinitionAcrossSuspend(*V, DVI))
FrameData.Spills[V].push_back(DVI);
+ // Add the instructions which carry debug info that is in the frame.
+ for (DPValue *DPV : DPVs)
+ if (Checker.isDefinitionAcrossSuspend(*V, DPV->Marker->MarkedInstr))
+ FrameData.Spills[V].push_back(DPV->Marker->MarkedInstr);
}
LLVM_DEBUG(dumpSpills("Spills", FrameData.Spills));
diff --git a/llvm/lib/Transforms/Coroutines/CoroInternal.h b/llvm/lib/Transforms/Coroutines/CoroInternal.h
index 0856c4925cc5d..1022f7a2979f5 100644
--- a/llvm/lib/Transforms/Coroutines/CoroInternal.h
+++ b/llvm/lib/Transforms/Coroutines/CoroInternal.h
@@ -33,6 +33,10 @@ void replaceCoroFree(CoroIdInst *CoroId, bool Elide);
void salvageDebugInfo(
SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap,
DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool IsEntryPoint);
+/// See overload.
+void salvageDebugInfo(
+ SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, DPValue *DPV,
+ bool OptimizeFrame, bool UseEntryValue);
// Keeps data and helper functions for lowering coroutine intrinsics.
struct LowererBase {
@@ -240,10 +244,13 @@ struct LLVM_LIBRARY_VISIBILITY Shape {
return nullptr;
}
- Instruction *getInsertPtAfterFramePtr() const {
- if (auto *I = dyn_cast<Instruction>(FramePtr))
- return I->getNextNode();
- return &cast<Argument>(FramePtr)->getParent()->getEntryBlock().front();
+ BasicBlock::iterator getInsertPtAfterFramePtr() const {
+ if (auto *I = dyn_cast<Instruction>(FramePtr)) {
+ BasicBlock::iterator It = std::next(I->getIterator());
+ It.setHeadBit(true); // Copy pre-RemoveDIs behaviour.
+ return It;
+ }
+ return cast<Argument>(FramePtr)->getParent()->getEntryBlock().begin();
}
/// Allocate memory according to the rules of the active lowering.
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 244580f503d5b..504aa912ac266 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -726,11 +726,14 @@ static void replaceSwiftErrorOps(Function &F, coro::Shape &Shape,
/// Returns all DbgVariableIntrinsic in F.
static SmallVector<DbgVariableIntrinsic *, 8>
-collectDbgVariableIntrinsics(Function &F) {
+collectDbgVariableIntrinsics(Function &F, SmallVector<DPValue *> &DPValues) {
SmallVector<DbgVariableIntrinsic *, 8> Intrinsics;
- for (auto &I : instructions(F))
+ for (auto &I : instructions(F)) {
+ for (DPValue &DPV : I.getDbgValueRange())
+ DPValues.push_back(&DPV);
if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&I))
Intrinsics.push_back(DVI);
+ }
return Intrinsics;
}
@@ -739,8 +742,9 @@ void CoroCloner::replaceSwiftErrorOps() {
}
void CoroCloner::salvageDebugInfo() {
+ SmallVector<DPValue *> DPValues;
SmallVector<DbgVariableIntrinsic *, 8> Worklist =
- collectDbgVariableIntrinsics(*NewF);
+ collectDbgVariableIntrinsics(*NewF, DPValues);
SmallDenseMap<Argument *, AllocaInst *, 4> ArgToAllocaMap;
// Only 64-bit ABIs have a register we can refer to with the entry value.
@@ -749,6 +753,9 @@ void CoroCloner::salvageDebugInfo() {
for (DbgVariableIntrinsic *DVI : Worklist)
coro::salvageDebugInfo(ArgToAllocaMap, DVI, Shape.OptimizeFrame,
UseEntryValue);
+ for (DPValue *DPV : DPValues)
+ coro::salvageDebugInfo(ArgToAllocaMap, DPV, Shape.OptimizeFrame,
+ UseEntryValue);
// Remove all salvaged dbg.declare intrinsics that became
// either unreachable or stale due to the CoroSplit transformation.
@@ -757,7 +764,7 @@ void CoroCloner::salvageDebugInfo() {
return !isPotentiallyReachable(&NewF->getEntryBlock(), BB, nullptr,
&DomTree);
};
- for (DbgVariableIntrinsic *DVI : Worklist) {
+ auto RemoveOne = [&](auto *DVI) {
if (IsUnreachableBlock(DVI->getParent()))
DVI->eraseFromParent();
else if (isa_and_nonnull<AllocaInst>(DVI->getVariableLocationOp(0))) {
@@ -770,7 +777,9 @@ void CoroCloner::salvageDebugInfo() {
if (!Uses)
DVI->eraseFromParent();
}
- }
+ };
+ for_each(Worklist, RemoveOne);
+ for_each(DPValues, RemoveOne);
}
void CoroCloner::replaceEntryBlock() {
@@ -1243,7 +1252,7 @@ static void updateCoroFrame(coro::Shape &Shape, Function *ResumeFn,
Function *DestroyFn, Function *CleanupFn) {
assert(Shape.ABI == coro::ABI::Switch);
- IRBuilder<> Builder(Shape.getInsertPtAfterFramePtr());
+ IRBuilder<> Builder(&*Shape.getInsertPtAfterFramePtr());
auto *ResumeAddr = Builder.CreateStructGEP(
Shape.FrameTy, Shape.FramePtr, coro::Shape::SwitchFieldIndex::Resume,
@@ -2039,10 +2048,13 @@ splitCoroutine(Function &F, SmallVectorImpl<Function *> &Clones,
// original function. The Cloner has already salvaged debug info in the new
// coroutine funclets.
SmallDenseMap<Argument *, AllocaInst *, 4> ArgToAllocaMap;
- for (auto *DDI : collectDbgVariableIntrinsics(F))
+ SmallVector<DPValue *> DPValues;
+ for (auto *DDI : collectDbgVariableIntrinsics(F, DPValues))
coro::salvageDebugInfo(ArgToAllocaMap, DDI, Shape.OptimizeFrame,
false /*UseEntryValue*/);
-
+ for (DPValue *DPV : DPValues)
+ coro::salvageDebugInfo(ArgToAllocaMap, DPV, Shape.OptimizeFrame,
+ false /*UseEntryValue*/);
return Shape;
}
diff --git a/llvm/test/Transforms/Coroutin...
[truncated]
|
|
|
|
||
| void coro::salvageDebugInfo( | ||
| SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, | ||
| DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool UseEntryValue) { |
There was a problem hiding this comment.
this function assumes DVI is non null, let's express that at the API level by making it a reference
| @@ -33,6 +33,10 @@ void replaceCoroFree(CoroIdInst *CoroId, bool Elide); | |||
| void salvageDebugInfo( | |||
| SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, | |||
| DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool IsEntryPoint); | |||
| /// See overload. | |||
There was a problem hiding this comment.
I'm not sure I understand what this comment is trying to accomplish, I'd suggest removing it or writing something about the "DPValue" bit
| @@ -2896,6 +2960,43 @@ void coro::salvageDebugInfo( | |||
| } | |||
| } | |||
|
|
|||
| void coro::salvageDebugInfo( | |||
| SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, DPValue *DPV, | |||
There was a problem hiding this comment.
DPV is assumed to be non-null, let's express that at the API level by making it a reference
| @@ -726,11 +726,14 @@ static void replaceSwiftErrorOps(Function &F, coro::Shape &Shape, | |||
|
|
|||
| /// Returns all DbgVariableIntrinsic in F. | |||
| static SmallVector<DbgVariableIntrinsic *, 8> | |||
| collectDbgVariableIntrinsics(Function &F) { | |||
| collectDbgVariableIntrinsics(Function &F, SmallVector<DPValue *> &DPValues) { | |||
There was a problem hiding this comment.
It feels wrong to have an in parameter and a return type that are virtually the same thing.
I believe we should express this at the API more clearly by returning a pair of vectors, or make two separate functions
There was a problem hiding this comment.
SGTM. I think it'd be cleaner to have two separate functions, but that'd make it easier to accidentally ignore DPValues, so I've gone with the slightly more cumbersome returned pair.
| DBuilder.createExpression(), DILoc, | ||
| Shape.getInsertPtAfterFramePtr()); | ||
| if (UseNewDbgInfoFormat) { | ||
| DPValue *NewDPV = new DPValue(ValueAsMetadata::get(Shape.FramePtr), |
There was a problem hiding this comment.
I am out of the loop of the RemoveDIs project, who owns this memory? The BB?
There was a problem hiding this comment.
The BasicBlock owns it indirectly. It's tied to the lifetime of the block, while not actually being part of the Value hierarchy. The DPMarker owns the DPValue memory, and BasicBlock owns the DPMarkers. Just like instructions, eraseFromParent can be called to delete these.
| SmallDenseMap<Argument *, AllocaInst *, 4> &ArgToAllocaMap, | ||
| DbgVariableIntrinsic *DVI, bool OptimizeFrame, bool UseEntryValue) { | ||
| Function *F = DVI->getFunction(); | ||
| static std::optional<std::pair<Value *, DIExpression *>> |
There was a problem hiding this comment.
why do we need an optional here? Pointers are already expressing the possibility of failure. We can probably do one of two things:
- Use an
optional<pair<Value&, DIExpression &>> - Just return a pair of pointers
The first option is nice because it makes it evident that they can only fail together, but I suspect 2 might be easier to read
|
RemoveDIs stuff can be found here: https://discourse.llvm.org/t/rfc-instruction-api-changes-needed-to-eliminate-debug-intrinsics-from-ir/68939 , a high level-ish overview is in the EuroLLVM talk I did https://www.youtube.com/watch?v=fZgFTOuhEzc . That covers the high level design idea, less so the actual class hierachy. |
|
Thanks @felipepiovezan for the review.
@jmorse posted some links above. To briefly contextualise the classes used here, we have
We're aiming for identical output with/without the new debug info mode - the no-debug-instruction mode should be a drop-in replacement for debug-intrinsic mode. |
felipepiovezan
approved these changes
Dec 11, 2023
As part of the RemoveDIs project, transitioning to non-instruction debug info, all debug intrinsic handling code needs to be duplicated to handle DPValues. --try-experimental-debuginfo-iterators enables the new debug mode in tests if the CMake option has been enabled.
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.
As part of the RemoveDIs project, transitioning to non-instruction debug info, all debug intrinsic handling code needs to be duplicated to handle DPValues.
--try-experimental-debuginfo-iterators enables the new debug mode in tests if the CMake option has been enabled.
getInsertPtAfterFramePtrnow returns an iterator so we don't lose debug-info-communicating bits.Depends on #73500, #74090, #74091.