[Coroutines] Improve debugging and minor refactoring #104642
Conversation
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-coroutines Author: Tyler Nowicki (TylerNowicki) Changes
Full diff: https://github.com/llvm/llvm-project/pull/104642.diff 3 Files Affected:
diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
index d8e827e9cebcfe..13b6680264c87c 100644
--- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
@@ -203,7 +203,7 @@ void Lowerer::lowerEarlyIntrinsics(Function &F) {
if (auto *CII = cast<CoroIdInst>(&I)) {
if (CII->getInfo().isPreSplit()) {
assert(F.isPresplitCoroutine() &&
- "The frontend uses Swtich-Resumed ABI should emit "
+ "The frontend uses Switch-Resumed ABI should emit "
"\"presplitcoroutine\" attribute for the coroutine.");
setCannotDuplicate(CII);
CII->setCoroutineSelf();
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index 73e30ea00a0e29..be268a8ca0f456 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -6,7 +6,8 @@
//
//===----------------------------------------------------------------------===//
// This file contains classes used to discover if for a particular value
-// there from sue to definition that crosses a suspend block.
+// its definition preceeds and its uses follow a suspend block. This is
+// referred to as a suspend crossing value.
//
// Using the information discovered we form a Coroutine Frame structure to
// contain those values. All uses of those values are replaced with appropriate
@@ -52,6 +53,16 @@ extern cl::opt<bool> UseNewDbgInfoFormat;
enum { SmallVectorThreshold = 32 };
+static std::string getBasicBlockLabel(const BasicBlock *BB) {
+ if (BB->hasName())
+ return BB->getName().str();
+
+ std::string S;
+ raw_string_ostream OS(S);
+ BB->printAsOperand(OS, false);
+ return OS.str().substr(1);
+}
+
// Provides two way mapping between the blocks and numbers.
namespace {
class BlockToIndexMapping {
@@ -123,8 +134,9 @@ class SuspendCrossingInfo {
public:
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
- void dump() const;
- void dump(StringRef Label, BitVector const &BV) const;
+ void dump(const ReversePostOrderTraversal<Function *> &RPOT) const;
+ void dump(StringRef Label, BitVector const &BV,
+ const ReversePostOrderTraversal<Function *> &RPOT) const;
#endif
SuspendCrossingInfo(Function &F, coro::Shape &Shape);
@@ -207,21 +219,25 @@ class SuspendCrossingInfo {
} // end anonymous namespace
#if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP)
-LLVM_DUMP_METHOD void SuspendCrossingInfo::dump(StringRef Label,
- BitVector const &BV) const {
+LLVM_DUMP_METHOD void SuspendCrossingInfo::dump(
+ StringRef Label, BitVector const &BV,
+ const ReversePostOrderTraversal<Function *> &RPOT) const {
dbgs() << Label << ":";
- for (size_t I = 0, N = BV.size(); I < N; ++I)
- if (BV[I])
- dbgs() << " " << Mapping.indexToBlock(I)->getName();
+ for (const BasicBlock *BB : RPOT) {
+ auto BBNo = Mapping.blockToIndex(BB);
+ if (BV[BBNo])
+ dbgs() << " " << getBasicBlockLabel(BB);
+ }
dbgs() << "\n";
}
-LLVM_DUMP_METHOD void SuspendCrossingInfo::dump() const {
- for (size_t I = 0, N = Block.size(); I < N; ++I) {
- BasicBlock *const B = Mapping.indexToBlock(I);
- dbgs() << B->getName() << ":\n";
- dump(" Consumes", Block[I].Consumes);
- dump(" Kills", Block[I].Kills);
+LLVM_DUMP_METHOD void SuspendCrossingInfo::dump(
+ const ReversePostOrderTraversal<Function *> &RPOT) const {
+ for (const BasicBlock *BB : RPOT) {
+ auto BBNo = Mapping.blockToIndex(BB);
+ dbgs() << getBasicBlockLabel(BB) << ":\n";
+ dump(" Consumes", Block[BBNo].Consumes, RPOT);
+ dump(" Kills", Block[BBNo].Kills, RPOT);
}
dbgs() << "\n";
}
@@ -335,7 +351,7 @@ SuspendCrossingInfo::SuspendCrossingInfo(Function &F, coro::Shape &Shape)
while (computeBlockData</*Initialize*/ false>(RPOT))
;
- LLVM_DEBUG(dump());
+ LLVM_DEBUG(dump(RPOT));
}
namespace {
@@ -419,7 +435,7 @@ struct RematGraph {
void dump() const {
dbgs() << "Entry (";
if (EntryNode->Node->getParent()->hasName())
- dbgs() << EntryNode->Node->getParent()->getName();
+ dbgs() << getBasicBlockLabel(EntryNode->Node->getParent());
else
EntryNode->Node->getParent()->printAsOperand(dbgs(), false);
dbgs() << ") : " << *EntryNode->Node << "\n";
@@ -551,7 +567,7 @@ struct FrameDataInfo {
#ifndef NDEBUG
static void dumpSpills(StringRef Title, const SpillInfo &Spills) {
- dbgs() << "------------- " << Title << "--------------\n";
+ dbgs() << "------------- " << Title << " --------------\n";
for (const auto &E : Spills) {
E.first->dump();
dbgs() << " user: ";
@@ -813,7 +829,7 @@ void FrameTypeBuilder::addFieldForAllocas(const Function &F,
StackLifetime StackLifetimeAnalyzer(F, ExtractAllocas(),
StackLifetime::LivenessType::May);
StackLifetimeAnalyzer.run();
- auto IsAllocaInferenre = [&](const AllocaInst *AI1, const AllocaInst *AI2) {
+ auto DoAllocasInterfere = [&](const AllocaInst *AI1, const AllocaInst *AI2) {
return StackLifetimeAnalyzer.getLiveRange(AI1).overlaps(
StackLifetimeAnalyzer.getLiveRange(AI2));
};
@@ -833,13 +849,13 @@ void FrameTypeBuilder::addFieldForAllocas(const Function &F,
for (const auto &A : FrameData.Allocas) {
AllocaInst *Alloca = A.Alloca;
bool Merged = false;
- // Try to find if the Alloca is not inferenced with any existing
+ // Try to find if the Alloca does not interfere with any existing
// NonOverlappedAllocaSet. If it is true, insert the alloca to that
// NonOverlappedAllocaSet.
for (auto &AllocaSet : NonOverlapedAllocas) {
assert(!AllocaSet.empty() && "Processing Alloca Set is not empty.\n");
- bool NoInference = none_of(AllocaSet, [&](auto Iter) {
- return IsAllocaInferenre(Alloca, Iter);
+ bool NoInterference = none_of(AllocaSet, [&](auto Iter) {
+ return DoAllocasInterfere(Alloca, Iter);
});
// If the alignment of A is multiple of the alignment of B, the address
// of A should satisfy the requirement for aligning for B.
@@ -852,7 +868,7 @@ void FrameTypeBuilder::addFieldForAllocas(const Function &F,
return LargestAlloca->getAlign().value() % Alloca->getAlign().value() ==
0;
}();
- bool CouldMerge = NoInference && Alignable;
+ bool CouldMerge = NoInterference && Alignable;
if (!CouldMerge)
continue;
AllocaSet.push_back(Alloca);
@@ -1730,6 +1746,51 @@ static Instruction *splitBeforeCatchSwitch(CatchSwitchInst *CatchSwitch) {
return CleanupRet;
}
+static BasicBlock::iterator getSpillInsertionPt(const coro::Shape &Shape,
+ Value *Def,
+ const DominatorTree &DT) {
+ BasicBlock::iterator InsertPt;
+ 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();
+
+ // If we're spilling an Argument, make sure we clear 'nocapture'
+ // from the coroutine function.
+ Arg->getParent()->removeParamAttr(Arg->getArgNo(), Attribute::NoCapture);
+ } else if (auto *CSI = dyn_cast<AnyCoroSuspendInst>(Def)) {
+ // Don't spill immediately after a suspend; splitting assumes
+ // that the suspend will be followed by a branch.
+ InsertPt = CSI->getParent()->getSingleSuccessor()->getFirstNonPHIIt();
+ } else {
+ auto *I = cast<Instruction>(Def);
+ if (!DT.dominates(Shape.CoroBegin, I)) {
+ // If it is not dominated by CoroBegin, then spill should be
+ // inserted immediately after CoroFrame is computed.
+ 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.
+ auto *NewBB = SplitEdge(II->getParent(), II->getNormalDest());
+ InsertPt = NewBB->getTerminator()->getIterator();
+ } else if (isa<PHINode>(I)) {
+ // Skip the PHINodes and EH pads instructions.
+ BasicBlock *DefBlock = I->getParent();
+ if (auto *CSI = dyn_cast<CatchSwitchInst>(DefBlock->getTerminator()))
+ InsertPt = splitBeforeCatchSwitch(CSI)->getIterator();
+ else
+ InsertPt = DefBlock->getFirstInsertionPt();
+ } else {
+ assert(!I->isTerminator() && "unexpected terminator");
+ // For all other values, the spill is placed immediately after
+ // the definition.
+ InsertPt = I->getNextNode()->getIterator();
+ }
+ }
+
+ return InsertPt;
+}
+
// Replace all alloca and SSA values that are accessed across suspend points
// with GetElementPointer from coroutine frame + loads and stores. Create an
// AllocaSpillBB that will become the new entry block for the resume parts of
@@ -1752,9 +1813,8 @@ static Instruction *splitBeforeCatchSwitch(CatchSwitchInst *CatchSwitch) {
//
//
static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
- auto *CB = Shape.CoroBegin;
- LLVMContext &C = CB->getContext();
- Function *F = CB->getFunction();
+ LLVMContext &C = Shape.CoroBegin->getContext();
+ Function *F = Shape.CoroBegin->getFunction();
IRBuilder<> Builder(C);
StructType *FrameTy = Shape.FrameTy;
Value *FramePtr = Shape.FramePtr;
@@ -1815,47 +1875,16 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
auto SpillAlignment = Align(FrameData.getAlign(Def));
// Create a store instruction storing the value into the
// coroutine frame.
- BasicBlock::iterator InsertPt;
+ BasicBlock::iterator InsertPt = getSpillInsertionPt(Shape, Def, DT);
+
Type *ByValTy = nullptr;
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();
-
// If we're spilling an Argument, make sure we clear 'nocapture'
// from the coroutine function.
Arg->getParent()->removeParamAttr(Arg->getArgNo(), Attribute::NoCapture);
if (Arg->hasByValAttr())
ByValTy = Arg->getParamByValType();
- } else if (auto *CSI = dyn_cast<AnyCoroSuspendInst>(Def)) {
- // Don't spill immediately after a suspend; splitting assumes
- // that the suspend will be followed by a branch.
- InsertPt = CSI->getParent()->getSingleSuccessor()->getFirstNonPHIIt();
- } else {
- auto *I = cast<Instruction>(Def);
- 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();
- } 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.
- auto *NewBB = SplitEdge(II->getParent(), II->getNormalDest());
- InsertPt = NewBB->getTerminator()->getIterator();
- } else if (isa<PHINode>(I)) {
- // Skip the PHINodes and EH pads instructions.
- BasicBlock *DefBlock = I->getParent();
- if (auto *CSI = dyn_cast<CatchSwitchInst>(DefBlock->getTerminator()))
- InsertPt = splitBeforeCatchSwitch(CSI)->getIterator();
- else
- InsertPt = DefBlock->getFirstInsertionPt();
- } else {
- assert(!I->isTerminator() && "unexpected terminator");
- // For all other values, the spill is placed immediately after
- // the definition.
- InsertPt = I->getNextNode()->getIterator();
- }
}
auto Index = FrameData.getFieldIndex(Def);
@@ -1998,7 +2027,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
UsersToUpdate.clear();
for (User *U : Alloca->users()) {
auto *I = cast<Instruction>(U);
- if (DT.dominates(CB, I))
+ if (DT.dominates(Shape.CoroBegin, I))
UsersToUpdate.push_back(I);
}
if (UsersToUpdate.empty())
@@ -2040,7 +2069,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
Builder.CreateStore(Value, G);
}
// For each alias to Alloca created before CoroBegin but used after
- // CoroBegin, we recreate them after CoroBegin by appplying the offset
+ // CoroBegin, we recreate them after CoroBegin by applying the offset
// to the pointer in the frame.
for (const auto &Alias : A.Aliases) {
auto *FramePtr = GetFramePointer(Alloca);
@@ -2049,7 +2078,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
auto *AliasPtr =
Builder.CreatePtrAdd(FramePtr, ConstantInt::get(ITy, Value));
Alias.first->replaceUsesWithIf(
- AliasPtr, [&](Use &U) { return DT.dominates(CB, U); });
+ AliasPtr, [&](Use &U) { return DT.dominates(Shape.CoroBegin, U); });
}
}
@@ -2062,7 +2091,7 @@ static void insertSpills(const FrameDataInfo &FrameData, coro::Shape &Shape) {
// If there is memory accessing to promise alloca before CoroBegin;
bool HasAccessingPromiseBeforeCB = llvm::any_of(PA->uses(), [&](Use &U) {
auto *Inst = dyn_cast<Instruction>(U.getUser());
- if (!Inst || DT.dominates(CB, Inst))
+ if (!Inst || DT.dominates(Shape.CoroBegin, Inst))
return false;
if (auto *CI = dyn_cast<CallInst>(Inst)) {
@@ -2105,8 +2134,9 @@ static void movePHIValuesToInsertedBlock(BasicBlock *SuccBB,
do {
int Index = PN->getBasicBlockIndex(InsertedBB);
Value *V = PN->getIncomingValue(Index);
- PHINode *InputV = PHINode::Create(
- V->getType(), 1, V->getName() + Twine(".") + SuccBB->getName());
+ PHINode *InputV =
+ PHINode::Create(V->getType(), 1,
+ V->getName() + Twine(".") + getBasicBlockLabel(SuccBB));
InputV->insertBefore(InsertedBB->begin());
InputV->addIncoming(V, PredBB);
PN->setIncomingValue(Index, InputV);
@@ -2151,10 +2181,10 @@ static void rewritePHIsForCleanupPad(BasicBlock *CleanupPadBB,
Builder.CreateUnreachable();
// Create a new cleanuppad which will be the dispatcher.
- auto *NewCleanupPadBB =
- BasicBlock::Create(CleanupPadBB->getContext(),
- CleanupPadBB->getName() + Twine(".corodispatch"),
- CleanupPadBB->getParent(), CleanupPadBB);
+ auto *NewCleanupPadBB = BasicBlock::Create(
+ CleanupPadBB->getContext(),
+ getBasicBlockLabel(CleanupPadBB) + Twine(".corodispatch"),
+ CleanupPadBB->getParent(), CleanupPadBB);
Builder.SetInsertPoint(NewCleanupPadBB);
auto *SwitchType = Builder.getInt8Ty();
auto *SetDispatchValuePN =
@@ -2168,13 +2198,14 @@ static void rewritePHIsForCleanupPad(BasicBlock *CleanupPadBB,
SmallVector<BasicBlock *, 8> Preds(predecessors(CleanupPadBB));
for (BasicBlock *Pred : Preds) {
// Create a new cleanuppad and move the PHI values to there.
- auto *CaseBB = BasicBlock::Create(CleanupPadBB->getContext(),
- CleanupPadBB->getName() +
- Twine(".from.") + Pred->getName(),
- CleanupPadBB->getParent(), CleanupPadBB);
+ auto *CaseBB =
+ BasicBlock::Create(CleanupPadBB->getContext(),
+ getBasicBlockLabel(CleanupPadBB) + Twine(".from.") +
+ getBasicBlockLabel(Pred),
+ CleanupPadBB->getParent(), CleanupPadBB);
updatePhiNodes(CleanupPadBB, Pred, CaseBB);
- CaseBB->setName(CleanupPadBB->getName() + Twine(".from.") +
- Pred->getName());
+ CaseBB->setName(getBasicBlockLabel(CleanupPadBB) + Twine(".from.") +
+ getBasicBlockLabel(Pred));
Builder.SetInsertPoint(CaseBB);
Builder.CreateBr(CleanupPadBB);
movePHIValuesToInsertedBlock(CleanupPadBB, CaseBB, NewCleanupPadBB);
@@ -2264,7 +2295,8 @@ static void rewritePHIs(BasicBlock &BB) {
SmallVector<BasicBlock *, 8> Preds(predecessors(&BB));
for (BasicBlock *Pred : Preds) {
auto *IncomingBB = ehAwareSplitEdge(Pred, &BB, LandingPad, ReplPHI);
- IncomingBB->setName(BB.getName() + Twine(".from.") + Pred->getName());
+ IncomingBB->setName(getBasicBlockLabel(&BB) + Twine(".from.") +
+ getBasicBlockLabel(Pred));
// Stop the moving of values at ReplPHI, as this is either null or the PHI
// that replaced the landing pad.
@@ -2708,7 +2740,7 @@ static void eliminateSwiftError(Function &F, coro::Shape &Shape) {
}
}
-/// retcon and retcon.once conventions assume that all spill uses can be sunk
+/// Async and Retcon{Once} conventions assume that all spill uses can be sunk
/// after the coro.begin intrinsic.
static void sinkSpillUsesAfterCoroBegin(Function &F,
const FrameDataInfo &FrameData,
@@ -3142,7 +3174,7 @@ void coro::buildCoroutineFrame(
cleanupSinglePredPHIs(F);
// Transforms multi-edge PHI Nodes, so that any value feeding into a PHI will
- // never has its definition separated from the PHI by the suspend point.
+ // never have its definition separated from the PHI by the suspend point.
rewritePHIs(F);
// Build suspend crossing info.
@@ -3239,6 +3271,7 @@ void coro::buildCoroutineFrame(
Shape.FramePtr = Shape.CoroBegin;
// For now, this works for C++ programs only.
buildFrameDebugInfo(F, Shape, FrameData);
+ // Insert spills and reloads
insertSpills(FrameData, Shape);
lowerLocalAllocas(LocalAllocas, DeadInstructions);
diff --git a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
index 8eceaef59a1e1f..fab387c65ab41f 100644
--- a/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroSplit.cpp
@@ -1221,11 +1221,10 @@ static void postSplitCleanup(Function &F) {
// frame if possible.
static void handleNoSuspendCoroutine(coro::Shape &Shape) {
auto *CoroBegin = Shape.CoroBegin;
- auto *CoroId = CoroBegin->getId();
- auto *AllocInst = CoroId->getCoroAlloc();
switch (Shape.ABI) {
case coro::ABI::Switch: {
- auto SwitchId = cast<CoroIdInst>(CoroId);
+ auto SwitchId = Shape.getSwitchCoroId();
+ auto *AllocInst = SwitchId->getCoroAlloc();
coro::replaceCoroFree(SwitchId, /*Elide=*/AllocInst != nullptr);
if (AllocInst) {
IRBuilder<> Builder(AllocInst);
@@ -1689,7 +1688,7 @@ static void splitAsyncCoroutine(Function &F, coro::Shape &Shape,
auto &Context = F.getContext();
auto *Int8PtrTy = PointerType::getUnqual(Context);
- auto *Id = cast<CoroIdAsyncInst>(Shape.CoroBegin->getId());
+ auto *Id = Shape.getAsyncCoroId();
IRBuilder<> Builder(Id);
auto *FramePtr = Id->getStorage();
@@ -1783,7 +1782,7 @@ static void splitRetconCoroutine(Function &F, coro::Shape &Shape,
F.removeRetAttr(Attribute::NonNull);
// Allocate the frame.
- auto *Id = cast<AnyCoroIdRetconInst>(Shape.CoroBegin->getId());
+ auto *Id = Shape.getRetconCoroId();
Value *RawFramePtr;
if (Shape.RetconLowering.IsFrameInlineInStorage) {
RawFramePtr = Id->getStorage();
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Thanks for the change. However, I am not a big fan of the getBasicBlockLabel function for multiple reasons.
- returning an owned string from
getBasicBlockLabelis, in fact, a regression. Originally, we usedBasicBlock::getName()which returns aStringRef. - It's unclear to me in which cases
printAsOperandwould do better thanBasicBlock::getName().
Also see the comment on dump(). The title of this patch wants better debugging experience and I don't think removing the zero-argument overload of dump() does that.
| std::string S; | ||
| raw_string_ostream OS(S); | ||
| BB->printAsOperand(OS, false); | ||
| return OS.str().substr(1); |
There was a problem hiding this comment.
Not a fan of this way of getting the name. This seems counterintuitive. As the very least I need a comment here telling me what printAsOperand is going to do differently than BB->getName().
I am also against blanket changes from BasicBlock::getName() into getBasicBlockLabel. The benefit is unclear to me. Can you show me an example where you won't have a name for the basic block but this printAsOperand approach works well?
There was a problem hiding this comment.
I agree, I will remove it. But I want to point out that BB->getName returns an empty string if the BB has no name, same goes for Instructions. For example, BBs with labels like 1234: and Instructions with labels like %456 do not have a name. So when we create basic blocks with 'BB->getName() + Twine(".from.") + Pred)->getName()' in most cases that produces a BB called ".from."
There was a problem hiding this comment.
Under the hood, printAsOperand uses SlotTracker. (https://github.com/llvm/llvm-project/blob/main/llvm/lib/IR/AsmWriter.cpp#L2599-L2629)
It's not entirely clear to me that this slot number will be stable across passes. Looking at the code, it's printAsOperand creates the SlotTracker. For immediate dumping of the code this may work fine but if you do this again in another pass you may get a different number for the same BB because values can be inserted above.
There was a problem hiding this comment.
In addressing your concerns I realized why I added this in the first place (I made these changes a few months ago). When dumping if we use BB->getName and there is no name, it will not print anything. You are right it is not stable across passes, so it should only be used for dumping. I have made that correction. But when dumping it is important to use otherwise the output will not be understandable. For example, without getBasicBlockLabel the dump for debug-only=coro-suspend-crossing will produce
:
Consumes:
Kills:
:
Consumes:
Kills:
:
Consumes:
Kills:
:
Consumes:
Kills:
.from.:
Consumes: .from.
Kills:
.from.1:
Consumes: .from.1
Kills:
:
Consumes: .from. .from.1
Kills:
:
Consumes: .from. .from.1
Kills:
with getBasicBlockLabel we see
2:
Consumes: 2
Kills:
16:
Consumes: 2 16
Kills:
65:
Consumes: 2 16 65
Kills:
100:
Consumes: 2 16 65 100
Kills:
.from.:
Consumes: 2 16 65 100 .from.
Kills:
.from.1:
Consumes: 2 16 65 .from.1
Kills:
You can see there is a lot of missing information if we use getName...
| @@ -52,6 +53,16 @@ extern cl::opt<bool> UseNewDbgInfoFormat; | |||
|
|
|||
| enum { SmallVectorThreshold = 32 }; | |||
|
|
|||
| static std::string getBasicBlockLabel(const BasicBlock *BB) { | |||
| if (BB->hasName()) | |||
| return BB->getName().str(); | |||
There was a problem hiding this comment.
This function could have returned a non-owning StringRef. This .str() here creates a copy of the string.
There was a problem hiding this comment.
I don't see how it can return a StringRef and an std::string... Anyway, as I said above this is just for dumping purposes so it doesn't need to be that efficient.
There was a problem hiding this comment.
If we just use BB->printAsOperand(dbgs(), false) we can remove this need to go to a string. But that would result in a % being printed before the BB label. I find that a bit confusing because BB labels are not otherwise printed with a preceding %.
There was a problem hiding this comment.
For example, when print remat right now I realized it is using BB->printAsOperand(dbgs(), false) but that results in output like
Entry (%1122) : %1182 = call ...
So the % is a bit confusing there.
There was a problem hiding this comment.
I prefer it to print
Entry (1122) : %1182 = call ...
| @@ -123,8 +134,9 @@ class SuspendCrossingInfo { | |||
|
|
|||
| public: | |||
| #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) | |||
| void dump() const; | |||
| void dump(StringRef Label, BitVector const &BV) const; | |||
| void dump(const ReversePostOrderTraversal<Function *> &RPOT) const; | |||
There was a problem hiding this comment.
I am against not having a zero-argument dump overload.
One benefit of this dump() function not taking any parameter is that you can easily dump stuff in a debugger session. Now I will have to look into how to construct a ReversePostOrderTraversal, which may not be possible.
There was a problem hiding this comment.
I can add this as a second dump, keeping the dump with no args.
There was a problem hiding this comment.
Done, dump() now does not need args.
They are all related to improving debugging. |
You would probably still have an easier time getting the changes merged, if you would split them into separate reviews. E.g., the adjusted comments and renaming "inference -> "interference" are all clear wins. You could already get approvals and ship those parts, while the discussion on the naming of basic blocks is still ongoing |
* Fix comments in several places * Instead of using BB-getName() which will return an empty string if the BB has no name, getBasicBlockLabel will give the BBs label e.g. #### this makes the IR easier to read, otherwise we get lots of blocks with names like ".from.". * Use RPO when dumping SuspendCrossingInfo. Without this the dump order is determined by the ptr addresses and so is not consistent from run to run making IR diffs difficult to read. * Inference -> Interference * Pull the logic that determines insertion location out of insertSpills and into getSpillInsertionPt, to differentiate between these two operations. * Use Shape getters for CoroId instead of getting it manually. *
TylerNowicki
force-pushed
the
users/tylernowicki/coro-minor-improvements
branch
from
8534815 to
b3e4337
Compare
| dbgs() << EntryNode->Node->getParent()->getName(); | ||
| else | ||
| EntryNode->Node->getParent()->printAsOperand(dbgs(), false); | ||
| dbgs() << getBasicBlockLabel(EntryNode->Node->getParent()); |
There was a problem hiding this comment.
@yuxuanchen1997 I restricted use of getBasicBlockName to dump methods. Please notice that similar code already appeared in the dump method for RematGraph.
There was a problem hiding this comment.
My only concern was the unnecessary copy of basic block name. You can do something with a macro like
#define BB_NAME(BB) (BB->hasName() ? BB->getName() : StringRef(getBasicBlockLabel(BB)))
This uses lifetime extension rules to make the latter remain allocated while used as a temporary.
But this is not just ugly, but also unsafe to be stored to a variable. A better way to do this is through a type similar to a tagged union of StringRef and std::string.
There is probably no point in optimizing this for a single dump method. With these reasons, I am not against landing this now. But since you mentioned that RematGraph is also using similar patterns, there might be motivation to solve it systematically. (e.g. BasicBlock::getDebugName())
There was a problem hiding this comment.
FYI I have a change queued up to eliminate the str copies
| dbgs() << EntryNode->Node->getParent()->getName(); | ||
| else | ||
| EntryNode->Node->getParent()->printAsOperand(dbgs(), false); | ||
| dbgs() << getBasicBlockLabel(EntryNode->Node->getParent()); |
There was a problem hiding this comment.
My only concern was the unnecessary copy of basic block name. You can do something with a macro like
#define BB_NAME(BB) (BB->hasName() ? BB->getName() : StringRef(getBasicBlockLabel(BB)))
This uses lifetime extension rules to make the latter remain allocated while used as a temporary.
But this is not just ugly, but also unsafe to be stored to a variable. A better way to do this is through a type similar to a tagged union of StringRef and std::string.
There is probably no point in optimizing this for a single dump method. With these reasons, I am not against landing this now. But since you mentioned that RematGraph is also using similar patterns, there might be motivation to solve it systematically. (e.g. BasicBlock::getDebugName())
TylerNowicki
changed the title
No Functional Changes