Skip to content

[NFC][DebugInfo] Use iterator moveBefore at many call-sites #123583

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CGCoroutine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ struct CallCoroDelete final : public EHScopeStack::Cleanup {

// Get back to the block we were originally and move coro.free there.
auto *InsertPt = SaveInsertBlock->getTerminator();
CoroFree->moveBefore(InsertPt);
CoroFree->moveBefore(InsertPt->getIterator());
CGF.Builder.SetInsertPoint(InsertPt);

// Add if (auto *mem = coro.free) Deallocate;
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CGException.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1858,7 +1858,7 @@ Address CodeGenFunction::recoverAddrOfEscapedLocal(CodeGenFunction &ParentCGF,
"expected alloca or localrecover in parent LocalDeclMap");
RecoverCall = cast<llvm::CallInst>(ParentRecover->clone());
RecoverCall->setArgOperand(1, ParentFP);
RecoverCall->insertBefore(AllocaInsertPt);
RecoverCall->insertBefore(AllocaInsertPt->getIterator());
}

// Bitcast the variable, rename it, and insert it in the local decl map.
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/CodeGen/CGOpenMPRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1332,7 +1332,7 @@ void CGOpenMPRuntime::setLocThreadIdInsertPt(CodeGenFunction &CGF,
CGF.Builder.GetInsertBlock());
} else {
Elem.ServiceInsertPt = new llvm::BitCastInst(Undef, CGF.Int32Ty, "svcpt");
Elem.ServiceInsertPt->insertAfter(CGF.AllocaInsertPt);
Elem.ServiceInsertPt->insertAfter(CGF.AllocaInsertPt->getIterator());
}
}

Expand Down
1 change: 1 addition & 0 deletions llvm/include/llvm/IR/BasicBlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
// debug-info attachments.
friend void Instruction::insertBefore(BasicBlock::iterator InsertPos);
friend void Instruction::insertAfter(Instruction *InsertPos);
friend void Instruction::insertAfter(BasicBlock::iterator InsertPos);
friend void Instruction::insertBefore(BasicBlock &BB,
InstListType::iterator InsertPos);
friend void Instruction::moveBeforeImpl(BasicBlock &BB,
Expand Down
8 changes: 8 additions & 0 deletions llvm/include/llvm/IR/DebugProgramInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -192,11 +192,19 @@ class DbgRecord : public ilist_node<DbgRecord> {

DbgRecord *getNextNode() { return &*std::next(getIterator()); }
DbgRecord *getPrevNode() { return &*std::prev(getIterator()); }

// Some generic lambdas supporting intrinsic-based debug-info mean we need
// to support both iterator and instruction position based insertion.
void insertBefore(DbgRecord *InsertBefore);
void insertAfter(DbgRecord *InsertAfter);
void moveBefore(DbgRecord *MoveBefore);
void moveAfter(DbgRecord *MoveAfter);

void insertBefore(self_iterator InsertBefore);
void insertAfter(self_iterator InsertAfter);
void moveBefore(self_iterator MoveBefore);
void moveAfter(self_iterator MoveAfter);

DebugLoc getDebugLoc() const { return DbgLoc; }
void setDebugLoc(DebugLoc Loc) { DbgLoc = std::move(Loc); }

Expand Down
23 changes: 20 additions & 3 deletions llvm/include/llvm/IR/Instruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -207,12 +207,19 @@ class Instruction : public User,
/// Insert an unlinked instruction into a basic block immediately before
/// the specified instruction.
void insertBefore(Instruction *InsertPos);

/// Insert an unlinked instruction into a basic block immediately before
/// the specified position.
void insertBefore(InstListType::iterator InsertPos);

/// Insert an unlinked instruction into a basic block immediately after the
/// specified instruction.
void insertAfter(Instruction *InsertPos);

/// Insert an unlinked instruction into a basic block immediately after the
/// specified position.
void insertAfter(InstListType::iterator InsertPos);

/// Inserts an unlinked instruction into \p ParentBB at position \p It and
/// returns the iterator of the inserted instruction.
InstListType::iterator insertInto(BasicBlock *ParentBB,
Expand All @@ -224,11 +231,15 @@ class Instruction : public User,
/// the basic block that MovePos lives in, right before MovePos.
void moveBefore(Instruction *MovePos);

/// Unlink this instruction from its current basic block and insert it into
/// the basic block that MovePos lives in, right before MovePos.
void moveBefore(InstListType::iterator InsertPos);
Comment on lines +235 to +236
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// the basic block that MovePos lives in, right before MovePos.
void moveBefore(InstListType::iterator InsertPos);
/// the basic block that MovePos lives in, right before MovePos.
void moveBefore(InstListType::iterator MovePos);

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: parameter name and comment don't match up


/// Perform a \ref moveBefore operation, while signalling that the caller
/// intends to preserve the original ordering of instructions. This implicitly
/// means that any adjacent debug-info should move with this instruction.
/// This method is currently a no-op placeholder, but it will become meaningful
/// when the "RemoveDIs" project is enabled.
/// This method is currently a no-op placeholder, but it will become
/// meaningful when the "RemoveDIs" project is enabled.
Comment on lines +241 to +242
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 comment stale?

void moveBeforePreserving(Instruction *MovePos);

private:
Expand All @@ -242,13 +253,19 @@ class Instruction : public User,
/// \pre I is a valid iterator into BB.
void moveBefore(BasicBlock &BB, InstListType::iterator I);

/// (See other overload for moveBeforePreserving).
void moveBeforePreserving(BasicBlock &BB, InstListType::iterator I);
/// Unlink this instruction from its current basic block and insert it into
/// the basic block that MovePos lives in, right before MovePos.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: mismatch param name in comment

void moveBeforePreserving(InstListType::iterator I);

/// Unlink this instruction from its current basic block and insert it into
/// the basic block that MovePos lives in, right after MovePos.
void moveAfter(Instruction *MovePos);

/// Unlink this instruction from its current basic block and insert it into
/// the basic block that MovePos lives in, right after MovePos.
void moveAfter(InstListType::iterator MovePos);

/// See \ref moveBeforePreserving .
void moveAfterPreserving(Instruction *MovePos);

Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/Analysis/LoopInfo.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ bool Loop::makeLoopInvariant(Instruction *I, bool &Changed,
return false;

// Hoist.
I->moveBefore(InsertPt);
I->moveBefore(InsertPt->getIterator());
if (MSSAU)
if (auto *MUD = MSSAU->getMemorySSA()->getMemoryAccess(I))
MSSAU->moveToPlace(MUD, InsertPt->getParent(),
Expand Down
32 changes: 16 additions & 16 deletions llvm/lib/CodeGen/CodeGenPrepare.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1264,7 +1264,7 @@ simplifyRelocatesOffABase(GCRelocateInst *RelocatedBase,
if (auto *RI = dyn_cast<GCRelocateInst>(R))
if (RI->getStatepoint() == RelocatedBase->getStatepoint())
if (RI->getBasePtrIndex() == RelocatedBase->getBasePtrIndex()) {
RelocatedBase->moveBefore(RI);
RelocatedBase->moveBefore(RI->getIterator());
MadeChange = true;
break;
}
Expand Down Expand Up @@ -2690,7 +2690,7 @@ bool CodeGenPrepare::optimizeCallInst(CallInst *CI, ModifyDT &ModifiedDT) {
ExtVal->getParent() == CI->getParent())
return false;
// Sink a zext feeding stlxr/stxr before it, so it can be folded into it.
ExtVal->moveBefore(CI);
ExtVal->moveBefore(CI->getIterator());
// Mark this instruction as "inserted by CGP", so that other
// optimizations don't touch it.
InsertedInsts.insert(ExtVal);
Expand Down Expand Up @@ -3036,7 +3036,7 @@ bool CodeGenPrepare::dupRetToEnableTailCallOpts(BasicBlock *BB,
for (auto *CI : CallInsts) {
for (auto const *FakeUse : FakeUses) {
auto *ClonedInst = FakeUse->clone();
ClonedInst->insertBefore(CI);
ClonedInst->insertBefore(CI->getIterator());
}
}
BB->eraseFromParent();
Expand Down Expand Up @@ -7552,9 +7552,9 @@ bool CodeGenPrepare::optimizeSelectInst(SelectInst *SI) {
// Sink expensive instructions into the conditional blocks to avoid executing
// them speculatively.
for (Instruction *I : TrueInstrs)
I->moveBefore(TrueBranch);
I->moveBefore(TrueBranch->getIterator());
for (Instruction *I : FalseInstrs)
I->moveBefore(FalseBranch);
I->moveBefore(FalseBranch->getIterator());

// If we did not create a new block for one of the 'true' or 'false' paths
// of the condition, it means that side of the branch goes to the end block
Expand Down Expand Up @@ -7682,7 +7682,7 @@ bool CodeGenPrepare::tryToSinkFreeOperands(Instruction *I) {
NewInstructions[UI] = NI;
MaybeDead.insert(UI);
LLVM_DEBUG(dbgs() << "Sinking " << *UI << " to user " << *I << "\n");
NI->insertBefore(InsertPoint);
NI->insertBefore(InsertPoint->getIterator());
InsertPoint = NI;
InsertedInsts.insert(NI);

Expand Down Expand Up @@ -7744,7 +7744,7 @@ bool CodeGenPrepare::optimizeSwitchType(SwitchInst *SI) {
}

auto *ExtInst = CastInst::Create(ExtType, Cond, NewType);
ExtInst->insertBefore(SI);
ExtInst->insertBefore(SI->getIterator());
ExtInst->setDebugLoc(SI->getDebugLoc());
SI->setCondition(ExtInst);
for (auto Case : SI->cases()) {
Expand Down Expand Up @@ -8556,7 +8556,7 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI,
match(UI, m_Shr(m_Specific(X), m_SpecificInt(CmpC.logBase2())))) {
IRBuilder<> Builder(Branch);
if (UI->getParent() != Branch->getParent())
UI->moveBefore(Branch);
UI->moveBefore(Branch->getIterator());
UI->dropPoisonGeneratingFlags();
Value *NewCmp = Builder.CreateCmp(ICmpInst::ICMP_EQ, UI,
ConstantInt::get(UI->getType(), 0));
Expand All @@ -8570,7 +8570,7 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI,
match(UI, m_Sub(m_Specific(X), m_SpecificInt(CmpC))))) {
IRBuilder<> Builder(Branch);
if (UI->getParent() != Branch->getParent())
UI->moveBefore(Branch);
UI->moveBefore(Branch->getIterator());
UI->dropPoisonGeneratingFlags();
Value *NewCmp = Builder.CreateCmp(Cmp->getPredicate(), UI,
ConstantInt::get(UI->getType(), 0));
Expand Down Expand Up @@ -8890,21 +8890,21 @@ bool CodeGenPrepare::fixupDbgVariableRecord(DbgVariableRecord &DVR) {
return AnyChange;
}

static void DbgInserterHelper(DbgValueInst *DVI, Instruction *VI) {
static void DbgInserterHelper(DbgValueInst *DVI, BasicBlock::iterator VI) {
DVI->removeFromParent();
if (isa<PHINode>(VI))
DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
DVI->insertBefore(VI->getParent()->getFirstInsertionPt());
else
DVI->insertAfter(VI);
}

static void DbgInserterHelper(DbgVariableRecord *DVR, Instruction *VI) {
static void DbgInserterHelper(DbgVariableRecord *DVR, BasicBlock::iterator VI) {
DVR->removeFromParent();
BasicBlock *VIBB = VI->getParent();
if (isa<PHINode>(VI))
VIBB->insertDbgRecordBefore(DVR, VIBB->getFirstInsertionPt());
else
VIBB->insertDbgRecordAfter(DVR, VI);
VIBB->insertDbgRecordAfter(DVR, &*VI);
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 a code-transition-state bug, or is there some reason we've got to use the Instruction * overload here? I suppose it doesn't matter either way for dbg records.

}

// A llvm.dbg.value may be using a value before its definition, due to
Expand Down Expand Up @@ -8954,7 +8954,7 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {

LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
<< *DbgItem << ' ' << *VI);
DbgInserterHelper(DbgItem, VI);
DbgInserterHelper(DbgItem, VI->getIterator());
MadeChange = true;
++NumDbgValueMoved;
}
Expand Down Expand Up @@ -8997,7 +8997,7 @@ bool CodeGenPrepare::placePseudoProbes(Function &F) {
I++;
while (I != Block.end()) {
if (auto *II = dyn_cast<PseudoProbeInst>(I++)) {
II->moveBefore(&*FirstInst);
II->moveBefore(FirstInst);
MadeChange = true;
}
}
Expand Down Expand Up @@ -9105,7 +9105,7 @@ bool CodeGenPrepare::splitBranchCondition(Function &F, ModifyDT &ModifiedDT) {
auto *Br2 = IRBuilder<>(TmpBB).CreateCondBr(Cond2, TBB, FBB);
if (auto *I = dyn_cast<Instruction>(Cond2)) {
I->removeFromParent();
I->insertBefore(Br2);
I->insertBefore(Br2->getIterator());
}

// Update PHI nodes in both successors. The original BB needs to be
Expand Down
8 changes: 4 additions & 4 deletions llvm/lib/CodeGen/SelectOptimize.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ static Value *getTrueOrFalseValue(
CBO->setOperand(OtherIdx,
isTrue ? OptSelects[IV].first : OptSelects[IV].second);
}
CBO->insertBefore(B->getTerminator());
CBO->insertBefore(B->getTerminator()->getIterator());
return CBO;
}

Expand Down Expand Up @@ -637,7 +637,7 @@ void SelectOptimizeImpl::convertProfitableSIGroups(SelectGroups &ProfSIGroups) {
}
auto InsertionPoint = EndBlock->getFirstInsertionPt();
for (auto *DI : SinkInstrs)
DI->moveBeforePreserving(&*InsertionPoint);
DI->moveBeforePreserving(InsertionPoint);

// Duplicate implementation for DbgRecords, the non-instruction debug-info
// format. Helper lambda for moving DbgRecords to the end block.
Expand Down Expand Up @@ -675,7 +675,7 @@ void SelectOptimizeImpl::convertProfitableSIGroups(SelectGroups &ProfSIGroups) {
TrueBranch = BranchInst::Create(EndBlock, TrueBlock);
TrueBranch->setDebugLoc(LastSI.getI()->getDebugLoc());
for (Instruction *TrueInst : TrueSlicesInterleaved)
TrueInst->moveBefore(TrueBranch);
TrueInst->moveBefore(TrueBranch->getIterator());
}
if (!FalseSlicesInterleaved.empty() || HasSelectLike(ASI, false)) {
FalseBlock =
Expand All @@ -684,7 +684,7 @@ void SelectOptimizeImpl::convertProfitableSIGroups(SelectGroups &ProfSIGroups) {
FalseBranch = BranchInst::Create(EndBlock, FalseBlock);
FalseBranch->setDebugLoc(LastSI.getI()->getDebugLoc());
for (Instruction *FalseInst : FalseSlicesInterleaved)
FalseInst->moveBefore(FalseBranch);
FalseInst->moveBefore(FalseBranch->getIterator());
}
// If there was nothing to sink, then arbitrarily choose the 'false' side
// for a new input value to the PHI.
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/CodeGen/SjLjEHPrepare.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -368,7 +368,7 @@ void SjLjEHPrepareImpl::lowerAcrossUnwindEdges(Function &F,
DemotePHIToStack(PN);

// Move the landingpad instruction back to the top of the landing pad block.
LPI->moveBefore(&UnwindBlock->front());
LPI->moveBefore(UnwindBlock->begin());
}
}

Expand Down Expand Up @@ -478,7 +478,7 @@ bool SjLjEHPrepareImpl::setupEntryBlockAndCallSites(Function &F) {
continue;
}
Instruction *StackAddr = CallInst::Create(StackAddrFn, "sp");
StackAddr->insertAfter(&I);
StackAddr->insertAfter(I.getIterator());
new StoreInst(StackAddr, StackPtr, true,
std::next(StackAddr->getIterator()));
}
Expand Down
5 changes: 3 additions & 2 deletions llvm/lib/CodeGen/StackColoring.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,8 @@ void StackColoring::remapInstructions(DenseMap<int, int> &SlotRemap) {
// If From is before wo, its possible that there is a use of From between
// them.
if (From->comesBefore(To))
const_cast<AllocaInst*>(To)->moveBefore(const_cast<AllocaInst*>(From));
const_cast<AllocaInst *>(To)->moveBefore(
const_cast<AllocaInst *>(From)->getIterator());

// AA might be used later for instruction scheduling, and we need it to be
// able to deduce the correct aliasing releationships between pointers
Expand All @@ -948,7 +949,7 @@ void StackColoring::remapInstructions(DenseMap<int, int> &SlotRemap) {
Instruction *Inst = const_cast<AllocaInst *>(To);
if (From->getType() != To->getType()) {
BitCastInst *Cast = new BitCastInst(Inst, From->getType());
Cast->insertAfter(Inst);
Cast->insertAfter(Inst->getIterator());
Inst = Cast;
}

Expand Down
14 changes: 7 additions & 7 deletions llvm/lib/CodeGen/TypePromotion.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ void IRPromoter::ReplaceAllUsersOfWith(Value *From, Value *To) {
void IRPromoter::ExtendSources() {
IRBuilder<> Builder{Ctx};

auto InsertZExt = [&](Value *V, Instruction *InsertPt) {
auto InsertZExt = [&](Value *V, BasicBlock::iterator InsertPt) {
assert(V->getType() != ExtTy && "zext already extends to i32");
LLVM_DEBUG(dbgs() << "IR Promotion: Inserting ZExt for " << *V << "\n");
Builder.SetInsertPoint(InsertPt);
Expand All @@ -448,7 +448,7 @@ void IRPromoter::ExtendSources() {
if (isa<Argument>(V))
I->moveBefore(InsertPt);
else
I->moveAfter(InsertPt);
I->moveAfter(&*InsertPt);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as earlier - is this a code-transition-state bug, or is there some reason we've got to use the Instruction * overload here? (this one matters more)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question on this and the other item; I think the most important fact is that inserting after an instruction never introduces a problem with head-bits and debug records, because it's always "in front" of any debug records. As a result, it's not necessary to update moveAfter before stripping out intrinsic-support from LLVM (if we get there) so I haven't focused on this.

We could technically update moveAfter to use iterators (which has the benefit of uniformity); I suppose it's a transition matter because I'm not planning on doing it immediately.

NewInsts.insert(I);
}

Expand All @@ -460,10 +460,10 @@ void IRPromoter::ExtendSources() {
for (auto *V : Sources) {
LLVM_DEBUG(dbgs() << " - " << *V << "\n");
if (auto *I = dyn_cast<Instruction>(V))
InsertZExt(I, I);
InsertZExt(I, I->getIterator());
else if (auto *Arg = dyn_cast<Argument>(V)) {
BasicBlock &BB = Arg->getParent()->front();
InsertZExt(Arg, &*BB.getFirstInsertionPt());
InsertZExt(Arg, BB.getFirstInsertionPt());
} else {
llvm_unreachable("unhandled source that needs extending");
}
Expand Down Expand Up @@ -552,7 +552,7 @@ void IRPromoter::TruncateSinks() {
Value *Arg = Call->getArgOperand(i);
Type *Ty = TruncTysMap[Call][i];
if (Instruction *Trunc = InsertTrunc(Arg, Ty)) {
Trunc->moveBefore(Call);
Trunc->moveBefore(Call->getIterator());
Call->setArgOperand(i, Trunc);
}
}
Expand All @@ -563,7 +563,7 @@ void IRPromoter::TruncateSinks() {
if (auto *Switch = dyn_cast<SwitchInst>(I)) {
Type *Ty = TruncTysMap[Switch][0];
if (Instruction *Trunc = InsertTrunc(Switch->getCondition(), Ty)) {
Trunc->moveBefore(Switch);
Trunc->moveBefore(Switch->getIterator());
Switch->setCondition(Trunc);
}
continue;
Expand All @@ -583,7 +583,7 @@ void IRPromoter::TruncateSinks() {
for (unsigned i = 0; i < I->getNumOperands(); ++i) {
Type *Ty = TruncTysMap[I][i];
if (Instruction *Trunc = InsertTrunc(I->getOperand(i), Ty)) {
Trunc->moveBefore(I);
Trunc->moveBefore(I->getIterator());
I->setOperand(i, Trunc);
}
}
Expand Down
4 changes: 2 additions & 2 deletions llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1488,12 +1488,12 @@ OpenMPIRBuilder::InsertPointOrErrorTy OpenMPIRBuilder::createParallel(
// Add additional casts to enforce pointers in zero address space
TIDAddr = new AddrSpaceCastInst(
TIDAddrAlloca, PointerType ::get(M.getContext(), 0), "tid.addr.ascast");
TIDAddr->insertAfter(TIDAddrAlloca);
TIDAddr->insertAfter(TIDAddrAlloca->getIterator());
ToBeDeleted.push_back(TIDAddr);
ZeroAddr = new AddrSpaceCastInst(ZeroAddrAlloca,
PointerType ::get(M.getContext(), 0),
"zero.addr.ascast");
ZeroAddr->insertAfter(ZeroAddrAlloca);
ZeroAddr->insertAfter(ZeroAddrAlloca->getIterator());
ToBeDeleted.push_back(ZeroAddr);
}

Expand Down
Loading
Loading