diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h index a785a43a0f11a..cd814e21be1ae 100644 --- a/llvm/include/llvm/IR/Instruction.h +++ b/llvm/include/llvm/IR/Instruction.h @@ -90,6 +90,12 @@ class Instruction : public User, /// Returns true if any DPValues are attached to this instruction. bool hasDbgValues() const; + /// Transfer any DPValues on the position \p It onto this instruction, + /// by simply adopting the sequence of DPValues (which is efficient) if + /// possible, by merging two sequences otherwise. + void adoptDbgValues(BasicBlock *BB, InstListType::iterator It, + bool InsertAtHead); + /// Erase any DPValues attached to this instruction. void dropDbgValues(); diff --git a/llvm/lib/IR/BasicBlock.cpp b/llvm/lib/IR/BasicBlock.cpp index dca5283283847..358d6fd26cf10 100644 --- a/llvm/lib/IR/BasicBlock.cpp +++ b/llvm/lib/IR/BasicBlock.cpp @@ -81,8 +81,10 @@ void BasicBlock::convertToNewDbgValues() { continue; } - // Create a marker to store DPValues in. Technically we don't need to store - // one marker per instruction, but that's a future optimisation. + if (DPVals.empty()) + continue; + + // Create a marker to store DPValues in. createMarker(&I); DPMarker *Marker = I.DbgMarker; @@ -769,6 +771,7 @@ void BasicBlock::flushTerminatorDbgValues() { return; // Transfer DPValues from the trailing position onto the terminator. + createMarker(Term); Term->DbgMarker->absorbDebugValues(*TrailingDPValues, false); TrailingDPValues->eraseFromParent(); deleteTrailingDPValues(); @@ -812,10 +815,9 @@ void BasicBlock::spliceDebugInfoEmptyBlock(BasicBlock::iterator Dest, if (!SrcTrailingDPValues) return; - DPMarker *M = Dest->DbgMarker; - M->absorbDebugValues(*SrcTrailingDPValues, InsertAtHead); - SrcTrailingDPValues->eraseFromParent(); - Src->deleteTrailingDPValues(); + Dest->adoptDbgValues(Src, Src->end(), InsertAtHead); + // adoptDbgValues should have released the trailing DPValues. + assert(!Src->getTrailingDPValues()); return; } @@ -882,7 +884,6 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src, } if (First->hasDbgValues()) { - DPMarker *CurMarker = Src->getMarker(First); // Place them at the front, it would look like this: // Dest // | @@ -890,8 +891,7 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src, // Src-block: ~~~~~~~~++++B---B---B---B:::C // | | // First Last - CurMarker->absorbDebugValues(*OurTrailingDPValues, true); - OurTrailingDPValues->eraseFromParent(); + First->adoptDbgValues(this, end(), true); } else { // No current marker, create one and absorb in. (FIXME: we can avoid an // allocation in the future). @@ -911,7 +911,8 @@ void BasicBlock::spliceDebugInfo(BasicBlock::iterator Dest, BasicBlock *Src, if (!MoreDanglingDPValues) return; - // FIXME: we could avoid an allocation here sometimes. + // FIXME: we could avoid an allocation here sometimes. (adoptDbgValues + // requires an iterator). DPMarker *LastMarker = Src->createMarker(Last); LastMarker->absorbDebugValues(*MoreDanglingDPValues, true); MoreDanglingDPValues->eraseFromParent(); @@ -993,20 +994,22 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src, // Detach the marker at Dest -- this lets us move the "====" DPValues around. DPMarker *DestMarker = nullptr; if (Dest != end()) { - DestMarker = getMarker(Dest); - DestMarker->removeFromParent(); - createMarker(&*Dest); + if (DestMarker = getMarker(Dest)) + DestMarker->removeFromParent(); } // If we're moving the tail range of DPValues (":::"), absorb them into the // front of the DPValues at Dest. if (ReadFromTail && Src->getMarker(Last)) { - DPMarker *OntoDest = getMarker(Dest); DPMarker *FromLast = Src->getMarker(Last); - OntoDest->absorbDebugValues(*FromLast, true); if (LastIsEnd) { - FromLast->eraseFromParent(); - Src->deleteTrailingDPValues(); + Dest->adoptDbgValues(Src, Last, true); + // adoptDbgValues will release any trailers. + assert(!Src->getTrailingDPValues()); + } else { + // FIXME: can we use adoptDbgValues here to reduce allocations? + DPMarker *OntoDest = createMarker(Dest); + OntoDest->absorbDebugValues(*FromLast, true); } } @@ -1014,10 +1017,15 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src, // move their markers onto Last. They remain in the Src block. No action // needed. if (!ReadFromHead && First->hasDbgValues()) { - DPMarker *OntoLast = Src->createMarker(Last); - DPMarker *FromFirst = Src->createMarker(First); - OntoLast->absorbDebugValues(*FromFirst, - true); // Always insert at head of it. + DPMarker *FromFirst = Src->getMarker(First); + if (Last != Src->end()) { + Last->adoptDbgValues(Src, First, true); + } else { + DPMarker *OntoLast = Src->createMarker(Last); + DPMarker *FromFirst = Src->createMarker(First); + // Always insert at front of Last. + OntoLast->absorbDebugValues(*FromFirst, true); + } } // Finally, do something with the "====" DPValues we detached. @@ -1025,12 +1033,12 @@ void BasicBlock::spliceDebugInfoImpl(BasicBlock::iterator Dest, BasicBlock *Src, if (InsertAtHead) { // Insert them at the end of the DPValues at Dest. The "::::" DPValues // might be in front of them. - DPMarker *NewDestMarker = getMarker(Dest); + DPMarker *NewDestMarker = createMarker(Dest); NewDestMarker->absorbDebugValues(*DestMarker, false); } else { // Insert them right at the start of the range we moved, ahead of First // and the "++++" DPValues. - DPMarker *FirstMarker = getMarker(First); + DPMarker *FirstMarker = createMarker(First); FirstMarker->absorbDebugValues(*DestMarker, true); } DestMarker->eraseFromParent(); @@ -1082,9 +1090,7 @@ void BasicBlock::insertDPValueAfter(DPValue *DPV, Instruction *I) { assert(I->getParent() == this); iterator NextIt = std::next(I->getIterator()); - DPMarker *NextMarker = getMarker(NextIt); - if (!NextMarker) - NextMarker = createMarker(NextIt); + DPMarker *NextMarker = createMarker(NextIt); NextMarker->insertDPValue(DPV, true); } @@ -1097,6 +1103,7 @@ void BasicBlock::insertDPValueBefore(DPValue *DPV, if (!Where->DbgMarker) createMarker(Where); bool InsertAtHead = Where.getHeadBit(); + createMarker(&*Where); Where->DbgMarker->insertDPValue(DPV, InsertAtHead); } diff --git a/llvm/lib/IR/DebugProgramInstruction.cpp b/llvm/lib/IR/DebugProgramInstruction.cpp index 03085b3bfd127..1a62902115be1 100644 --- a/llvm/lib/IR/DebugProgramInstruction.cpp +++ b/llvm/lib/IR/DebugProgramInstruction.cpp @@ -434,13 +434,23 @@ void DPMarker::removeMarker() { // instruction. If there isn't a next instruction, put them on the // "trailing" list. DPMarker *NextMarker = Owner->getParent()->getNextMarker(Owner); - if (NextMarker == nullptr) { - NextMarker = new DPMarker(); - Owner->getParent()->setTrailingDPValues(NextMarker); + if (NextMarker) { + NextMarker->absorbDebugValues(*this, true); + eraseFromParent(); + } else { + // We can avoid a deallocation -- just store this marker onto the next + // instruction. Unless we're at the end of the block, in which case this + // marker becomes the trailing marker of a degenerate block. + BasicBlock::iterator NextIt = std::next(Owner->getIterator()); + if (NextIt == getParent()->end()) { + getParent()->setTrailingDPValues(this); + MarkedInstr = nullptr; + } else { + NextIt->DbgMarker = this; + MarkedInstr = &*NextIt; + } } - NextMarker->absorbDebugValues(*this, true); - - eraseFromParent(); + Owner->DbgMarker = nullptr; } void DPMarker::removeFromParent() { diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp index 904ce17fb0e7c..23a3e72da51d4 100644 --- a/llvm/lib/IR/Instruction.cpp +++ b/llvm/lib/IR/Instruction.cpp @@ -111,11 +111,6 @@ void Instruction::insertAfter(Instruction *InsertPos) { BasicBlock *DestParent = InsertPos->getParent(); DestParent->getInstList().insertAfter(InsertPos->getIterator(), this); - - // No need to manually update DPValues: if we insert after an instruction - // position, then we can never have any DPValues on "this". - if (DestParent->IsNewDbgInfoFormat) - DestParent->createMarker(this); } BasicBlock::iterator Instruction::insertInto(BasicBlock *ParentBB, @@ -138,17 +133,15 @@ void Instruction::insertBefore(BasicBlock &BB, if (!BB.IsNewDbgInfoFormat) return; - BB.createMarker(this); - // We've inserted "this": if InsertAtHead is set then it comes before any // DPValues attached to InsertPos. But if it's not set, then any DPValues // should now come before "this". bool InsertAtHead = InsertPos.getHeadBit(); if (!InsertAtHead) { DPMarker *SrcMarker = BB.getMarker(InsertPos); - // If there's no source marker, InsertPos is very likely end(). - if (SrcMarker) - DbgMarker->absorbDebugValues(*SrcMarker, false); + if (SrcMarker && !SrcMarker->empty()) { + adoptDbgValues(&BB, InsertPos, false); + } } // If we're inserting a terminator, check if we need to flush out @@ -212,14 +205,13 @@ void Instruction::moveBeforeImpl(BasicBlock &BB, InstListType::iterator I, BB.getInstList().splice(I, getParent()->getInstList(), getIterator()); if (BB.IsNewDbgInfoFormat && !Preserve) { - if (!DbgMarker) - BB.createMarker(this); DPMarker *NextMarker = getParent()->getNextMarker(this); // If we're inserting at point I, and not in front of the DPValues attached // there, then we should absorb the DPValues attached to I. - if (NextMarker && !InsertAtHead) - DbgMarker->absorbDebugValues(*NextMarker, false); + if (!InsertAtHead && NextMarker && !NextMarker->empty()) { + adoptDbgValues(&BB, I, false); + } } if (isTerminator()) @@ -258,6 +250,48 @@ std::optional Instruction::getDbgReinsertionPosition() { bool Instruction::hasDbgValues() const { return !getDbgValueRange().empty(); } +void Instruction::adoptDbgValues(BasicBlock *BB, BasicBlock::iterator It, + bool InsertAtHead) { + DPMarker *SrcMarker = BB->getMarker(It); + auto ReleaseTrailingDPValues = [BB, It, SrcMarker]() { + if (BB->end() == It) { + SrcMarker->eraseFromParent(); + BB->deleteTrailingDPValues(); + } + }; + + if (!SrcMarker || SrcMarker->StoredDPValues.empty()) { + ReleaseTrailingDPValues(); + return; + } + + // If we have DPMarkers attached to this instruction, we have to honour the + // ordering of DPValues between this and the other marker. Fall back to just + // absorbing from the source. + if (DbgMarker || It == BB->end()) { + // Ensure we _do_ have a marker. + getParent()->createMarker(this); + DbgMarker->absorbDebugValues(*SrcMarker, InsertAtHead); + + // Having transferred everything out of SrcMarker, we _could_ clean it up + // and free the marker now. However, that's a lot of heap-accounting for a + // small amount of memory with a good chance of re-use. Leave it for the + // moment. It will be released when the Instruction is freed in the worst + // case. + // However: if we transferred from a trailing marker off the end of the + // block, it's important to not leave the empty marker trailing. It will + // give a misleading impression that some debug records have been left + // trailing. + ReleaseTrailingDPValues(); + } else { + // Optimisation: we're transferring all the DPValues from the source marker + // onto this empty location: just adopt the other instructions marker. + DbgMarker = SrcMarker; + DbgMarker->MarkedInstr = this; + It->DbgMarker = nullptr; + } +} + void Instruction::dropDbgValues() { if (DbgMarker) DbgMarker->dropDPValues(); diff --git a/llvm/lib/Transforms/Utils/Local.cpp b/llvm/lib/Transforms/Utils/Local.cpp index 459e3d9805928..e4aa25f7ac6ad 100644 --- a/llvm/lib/Transforms/Utils/Local.cpp +++ b/llvm/lib/Transforms/Utils/Local.cpp @@ -2044,7 +2044,7 @@ static void insertDPValuesForPHIs(BasicBlock *BB, auto InsertionPt = Parent->getFirstInsertionPt(); assert(InsertionPt != Parent->end() && "Ill-formed basic block"); - InsertionPt->DbgMarker->insertDPValue(NewDbgII, true); + Parent->insertDPValueBefore(NewDbgII, InsertionPt); } } diff --git a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp index 504f4430dc2ca..ec59a07730203 100644 --- a/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp +++ b/llvm/lib/Transforms/Utils/LoopRotationUtils.cpp @@ -596,7 +596,10 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) { // on the next instruction, here labelled xyzzy, before we hoist %foo. // Later, we only only clone DPValues from that position (xyzzy) onwards, // which avoids cloning DPValue "blah" multiple times. - std::optional NextDbgInst = std::nullopt; + // (Stored as a range because it gives us a natural way of testing whether + // there were DPValues on the next instruction before we hoisted things). + iterator_range NextDbgInsts = + (I != E) ? I->getDbgValueRange() : DPMarker::getEmptyDPValueRange(); while (I != E) { Instruction *Inst = &*I++; @@ -611,9 +614,10 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) { !Inst->mayWriteToMemory() && !Inst->isTerminator() && !isa(Inst) && !isa(Inst)) { - if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat) { + if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat && + !NextDbgInsts.empty()) { auto DbgValueRange = - LoopEntryBranch->cloneDebugInfoFrom(Inst, NextDbgInst); + LoopEntryBranch->cloneDebugInfoFrom(Inst, NextDbgInsts.begin()); RemapDPValueRange(M, DbgValueRange, ValueMap, RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); // Erase anything we've seen before. @@ -622,7 +626,8 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) { DPV.eraseFromParent(); } - NextDbgInst = I->getDbgValueRange().begin(); + NextDbgInsts = I->getDbgValueRange(); + Inst->moveBefore(LoopEntryBranch); ++NumInstrsHoisted; @@ -635,11 +640,12 @@ bool LoopRotate::rotateLoop(Loop *L, bool SimplifiedLatch) { ++NumInstrsDuplicated; - if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat) { - auto Range = C->cloneDebugInfoFrom(Inst, NextDbgInst); + if (LoopEntryBranch->getParent()->IsNewDbgInfoFormat && + !NextDbgInsts.empty()) { + auto Range = C->cloneDebugInfoFrom(Inst, NextDbgInsts.begin()); RemapDPValueRange(M, Range, ValueMap, RF_NoModuleLevelChanges | RF_IgnoreMissingLocals); - NextDbgInst = std::nullopt; + NextDbgInsts = DPMarker::getEmptyDPValueRange(); // Erase anything we've seen before. for (DPValue &DPV : make_early_inc_range(Range)) if (DbgIntrinsics.count(makeHash(&DPV))) diff --git a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp index fb4847fc0a826..827b4a9c0cc32 100644 --- a/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp +++ b/llvm/unittests/IR/BasicBlockDbgInfoTest.cpp @@ -225,10 +225,9 @@ TEST(BasicBlockDbgInfoTest, MarkerOperations) { // then they would sit "above" the new instruction. Instr1->insertBefore(BB, BB.end()); EXPECT_EQ(Instr1->DbgMarker->StoredDPValues.size(), 2u); - // However we won't de-allocate the trailing marker until a terminator is - // inserted. - EXPECT_EQ(EndMarker->StoredDPValues.size(), 0u); - EXPECT_EQ(BB.getTrailingDPValues(), EndMarker); + // We should de-allocate the trailing marker when something is inserted + // at end(). + EXPECT_EQ(BB.getTrailingDPValues(), nullptr); // Remove Instr1: now the DPValues will fall down again, Instr1->removeFromParent(); @@ -394,12 +393,12 @@ TEST(BasicBlockDbgInfoTest, InstrDbgAccess) { Instruction *CInst = BInst->getNextNode(); Instruction *DInst = CInst->getNextNode(); - ASSERT_TRUE(BInst->DbgMarker); + ASSERT_FALSE(BInst->DbgMarker); ASSERT_TRUE(CInst->DbgMarker); ASSERT_EQ(CInst->DbgMarker->StoredDPValues.size(), 1u); DPValue *DPV1 = &*CInst->DbgMarker->StoredDPValues.begin(); ASSERT_TRUE(DPV1); - EXPECT_EQ(BInst->DbgMarker->StoredDPValues.size(), 0u); + EXPECT_FALSE(BInst->hasDbgValues()); // Clone DPValues from one inst to another. Other arguments to clone are // tested in DPMarker test.