[NFC][DebugInfo] Use iterator-flavour getFirstNonPHI at many call-sites #123737
Conversation
This file contains hidden or 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
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-backend-webassembly Author: Jeremy Morse (jmorse) Changes(NB: this will fail CI as it composes with #123583, but I'm not gluing them together or it'll be too hard to review). As part of the "RemoveDIs" project, BasicBlock::iterator now carries a debug-info bit that's needed when getFirstNonPHI and similar feed into instruction insertion positions. Call-sites where that's necessary were updated a year ago; but to ensure some type safety however, we'd like to have all calls to getFirstNonPHI use the iterator-returning version. This patch changes a bunch of call-sites calling getFirstNonPHI to use getFirstNonPHIIt, which returns an iterator. All these call sites are where it's obviously safe to fetch the iterator then dereference it. A follow-up patch will contain less-obviously-safe changes. We'll eventually deprecate and remove the instruction-pointer getFirstNonPHI, but not before adding concise documentation of what considerations are needed (very few). Patch is 88.59 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/123737.diff 64 Files Affected:
diff --git a/clang/lib/CodeGen/CGException.cpp b/clang/lib/CodeGen/CGException.cpp
index e7dd5fb01ebede..75f6f6c44e1fd9 100644
--- a/clang/lib/CodeGen/CGException.cpp
+++ b/clang/lib/CodeGen/CGException.cpp
@@ -1251,11 +1251,12 @@ void CodeGenFunction::ExitCXXTryStmt(const CXXTryStmt &S, bool IsFnTryBlock) {
llvm::BasicBlock *WasmCatchStartBlock = nullptr;
if (EHPersonality::get(*this).isWasmPersonality()) {
auto *CatchSwitch =
- cast<llvm::CatchSwitchInst>(DispatchBlock->getFirstNonPHI());
+ cast<llvm::CatchSwitchInst>(DispatchBlock->getFirstNonPHIIt());
WasmCatchStartBlock = CatchSwitch->hasUnwindDest()
? CatchSwitch->getSuccessor(1)
: CatchSwitch->getSuccessor(0);
- auto *CPI = cast<llvm::CatchPadInst>(WasmCatchStartBlock->getFirstNonPHI());
+ auto *CPI =
+ cast<llvm::CatchPadInst>(WasmCatchStartBlock->getFirstNonPHIIt());
CurrentFuncletPad = CPI;
}
@@ -2252,7 +2253,7 @@ void CodeGenFunction::ExitSEHTryStmt(const SEHTryStmt &S) {
// __except blocks don't get outlined into funclets, so immediately do a
// catchret.
llvm::CatchPadInst *CPI =
- cast<llvm::CatchPadInst>(CatchPadBB->getFirstNonPHI());
+ cast<llvm::CatchPadInst>(CatchPadBB->getFirstNonPHIIt());
llvm::BasicBlock *ExceptBB = createBasicBlock("__except");
Builder.CreateCatchRet(CPI, ExceptBB);
EmitBlock(ExceptBB);
diff --git a/clang/lib/CodeGen/MicrosoftCXXABI.cpp b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
index 90651c3bafe26e..0d53e8cb45fe77 100644
--- a/clang/lib/CodeGen/MicrosoftCXXABI.cpp
+++ b/clang/lib/CodeGen/MicrosoftCXXABI.cpp
@@ -918,7 +918,7 @@ void MicrosoftCXXABI::emitBeginCatch(CodeGenFunction &CGF,
VarDecl *CatchParam = S->getExceptionDecl();
llvm::BasicBlock *CatchPadBB = CGF.Builder.GetInsertBlock();
llvm::CatchPadInst *CPI =
- cast<llvm::CatchPadInst>(CatchPadBB->getFirstNonPHI());
+ cast<llvm::CatchPadInst>(CatchPadBB->getFirstNonPHIIt());
CGF.CurrentFuncletPad = CPI;
// If this is a catch-all or the catch parameter is unnamed, we don't need to
diff --git a/llvm/include/llvm/IR/BasicBlock.h b/llvm/include/llvm/IR/BasicBlock.h
index c7913e60cea083..a6cda04b39e605 100644
--- a/llvm/include/llvm/IR/BasicBlock.h
+++ b/llvm/include/llvm/IR/BasicBlock.h
@@ -672,7 +672,7 @@ class BasicBlock final : public Value, // Basic blocks are data objects also
void replaceSuccessorsPhiUsesWith(BasicBlock *New);
/// Return true if this basic block is an exception handling block.
- bool isEHPad() const { return getFirstNonPHI()->isEHPad(); }
+ bool isEHPad() const { return getFirstNonPHIIt()->isEHPad(); }
/// Return true if this basic block is a landing pad.
///
diff --git a/llvm/include/llvm/Transforms/Utils/Instrumentation.h b/llvm/include/llvm/Transforms/Utils/Instrumentation.h
index 4f67d079d14696..0e2c0d9bfa605a 100644
--- a/llvm/include/llvm/Transforms/Utils/Instrumentation.h
+++ b/llvm/include/llvm/Transforms/Utils/Instrumentation.h
@@ -204,6 +204,11 @@ struct InstrumentationIRBuilder : IRBuilder<> {
explicit InstrumentationIRBuilder(Instruction *IP) : IRBuilder<>(IP) {
ensureDebugInfo(*this, *IP->getFunction());
}
+
+ explicit InstrumentationIRBuilder(BasicBlock *BB, BasicBlock::iterator It)
+ : IRBuilder<>(BB, It) {
+ ensureDebugInfo(*this, *BB->getParent());
+ }
};
} // end namespace llvm
diff --git a/llvm/lib/Analysis/Loads.cpp b/llvm/lib/Analysis/Loads.cpp
index 11ccfa33821cad..9279f19b72a3f1 100644
--- a/llvm/lib/Analysis/Loads.cpp
+++ b/llvm/lib/Analysis/Loads.cpp
@@ -284,7 +284,7 @@ bool llvm::isDereferenceableAndAlignedInLoop(
DL.getTypeStoreSize(LI->getType()).getFixedValue());
const Align Alignment = LI->getAlign();
- Instruction *HeaderFirstNonPHI = L->getHeader()->getFirstNonPHI();
+ Instruction *HeaderFirstNonPHI = &*L->getHeader()->getFirstNonPHIIt();
// If given a uniform (i.e. non-varying) address, see if we can prove the
// access is safe within the loop w/o needing predication.
diff --git a/llvm/lib/Analysis/LoopNestAnalysis.cpp b/llvm/lib/Analysis/LoopNestAnalysis.cpp
index fe6d270b9ac53c..ead5cf610d9e11 100644
--- a/llvm/lib/Analysis/LoopNestAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopNestAnalysis.cpp
@@ -346,7 +346,7 @@ static bool checkLoopsStructure(const Loop &OuterLoop, const Loop &InnerLoop,
// "guarded" inner loop which contains "only" Phi nodes corresponding to the
// LCSSA Phi nodes in the exit block.
auto IsExtraPhiBlock = [&](const BasicBlock &BB) {
- return BB.getFirstNonPHI() == BB.getTerminator() &&
+ return &*BB.getFirstNonPHIIt() == BB.getTerminator() &&
all_of(BB.phis(), [&](const PHINode &PN) {
return all_of(PN.blocks(), [&](const BasicBlock *IncomingBlock) {
return IncomingBlock == InnerLoopExit ||
diff --git a/llvm/lib/Analysis/MustExecute.cpp b/llvm/lib/Analysis/MustExecute.cpp
index d5c665753075ce..fde6bbf9eb1817 100644
--- a/llvm/lib/Analysis/MustExecute.cpp
+++ b/llvm/lib/Analysis/MustExecute.cpp
@@ -275,7 +275,7 @@ bool SimpleLoopSafetyInfo::isGuaranteedToExecute(const Instruction &Inst,
// exit. At the moment, we use a (cheap) hack for the common case where
// the instruction of interest is the first one in the block.
return !HeaderMayThrow ||
- Inst.getParent()->getFirstNonPHIOrDbg() == &Inst;
+ &*Inst.getParent()->getFirstNonPHIOrDbg() == &Inst;
// If there is a path from header to exit or latch that doesn't lead to our
// instruction's block, return false.
diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp
index 6e2f0ebde9bb6c..3526253085e0b7 100644
--- a/llvm/lib/Analysis/ValueTracking.cpp
+++ b/llvm/lib/Analysis/ValueTracking.cpp
@@ -8239,7 +8239,7 @@ static bool programUndefinedIfUndefOrPoison(const Value *V,
if (!BB || !Visited.insert(BB).second)
break;
- Begin = BB->getFirstNonPHI()->getIterator();
+ Begin = BB->getFirstNonPHIIt();
End = BB->end();
}
return false;
diff --git a/llvm/lib/CodeGen/AsmPrinter/WinException.cpp b/llvm/lib/CodeGen/AsmPrinter/WinException.cpp
index 6d6432b61f2d7d..97b4a6a42d81db 100644
--- a/llvm/lib/CodeGen/AsmPrinter/WinException.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/WinException.cpp
@@ -928,8 +928,8 @@ void WinException::computeIP2StateTable(
BaseState = NullState;
StartLabel = Asm->getFunctionBegin();
} else {
- auto *FuncletPad =
- cast<FuncletPadInst>(FuncletStart->getBasicBlock()->getFirstNonPHI());
+ auto *FuncletPad = cast<FuncletPadInst>(
+ FuncletStart->getBasicBlock()->getFirstNonPHIIt());
assert(FuncInfo.FuncletBaseStateMap.count(FuncletPad) != 0);
BaseState = FuncInfo.FuncletBaseStateMap.find(FuncletPad)->second;
StartLabel = getMCSymbolForMBB(Asm, &*FuncletStart);
diff --git a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
index f668e41094bbc8..21622ea43724c1 100644
--- a/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
+++ b/llvm/lib/CodeGen/GlobalISel/IRTranslator.cpp
@@ -2866,7 +2866,7 @@ bool IRTranslator::findUnwindDestinations(
}
while (EHPadBB) {
- const Instruction *Pad = EHPadBB->getFirstNonPHI();
+ BasicBlock::const_iterator Pad = EHPadBB->getFirstNonPHIIt();
BasicBlock *NewEHPadBB = nullptr;
if (isa<LandingPadInst>(Pad)) {
// Stop on landingpads. They are not funclets.
@@ -2927,7 +2927,7 @@ bool IRTranslator::translateInvoke(const User &U,
return false;
// FIXME: support Windows exception handling.
- if (!isa<LandingPadInst>(EHPadBB->getFirstNonPHI()))
+ if (!isa<LandingPadInst>(EHPadBB->getFirstNonPHIIt()))
return false;
// FIXME: support Windows dllimport function calls and calls through
@@ -4031,7 +4031,7 @@ bool IRTranslator::runOnMachineFunction(MachineFunction &CurMF) {
MF->push_back(EntryBB);
EntryBuilder->setMBB(*EntryBB);
- DebugLoc DbgLoc = F.getEntryBlock().getFirstNonPHI()->getDebugLoc();
+ DebugLoc DbgLoc = F.getEntryBlock().getFirstNonPHIIt()->getDebugLoc();
SwiftError.setFunction(CurMF);
SwiftError.createEntriesInEntryBlock(DbgLoc);
diff --git a/llvm/lib/CodeGen/GlobalMerge.cpp b/llvm/lib/CodeGen/GlobalMerge.cpp
index 48d4d7848d84a7..a7b02aa663a194 100644
--- a/llvm/lib/CodeGen/GlobalMerge.cpp
+++ b/llvm/lib/CodeGen/GlobalMerge.cpp
@@ -632,7 +632,7 @@ void GlobalMergeImpl::setMustKeepGlobalVariables(Module &M) {
for (Function &F : M) {
for (BasicBlock &BB : F) {
- Instruction *Pad = BB.getFirstNonPHI();
+ BasicBlock::iterator Pad = BB.getFirstNonPHIIt();
auto *II = dyn_cast<IntrinsicInst>(Pad);
if (!Pad->isEHPad() &&
!(II && II->getIntrinsicID() == Intrinsic::eh_typeid_for))
diff --git a/llvm/lib/CodeGen/MachineFunction.cpp b/llvm/lib/CodeGen/MachineFunction.cpp
index b8dbe834a4d511..6196bcc478b6fe 100644
--- a/llvm/lib/CodeGen/MachineFunction.cpp
+++ b/llvm/lib/CodeGen/MachineFunction.cpp
@@ -833,7 +833,8 @@ MCSymbol *MachineFunction::addLandingPad(MachineBasicBlock *LandingPad) {
LandingPadInfo &LP = getOrCreateLandingPadInfo(LandingPad);
LP.LandingPadLabel = LandingPadLabel;
- const Instruction *FirstI = LandingPad->getBasicBlock()->getFirstNonPHI();
+ BasicBlock::const_iterator FirstI =
+ LandingPad->getBasicBlock()->getFirstNonPHIIt();
if (const auto *LPI = dyn_cast<LandingPadInst>(FirstI)) {
// If there's no typeid list specified, then "cleanup" is implicit.
// Otherwise, id 0 is reserved for the cleanup action.
diff --git a/llvm/lib/CodeGen/SelectOptimize.cpp b/llvm/lib/CodeGen/SelectOptimize.cpp
index bfc49dd354aa60..0f5bdea2f12664 100644
--- a/llvm/lib/CodeGen/SelectOptimize.cpp
+++ b/llvm/lib/CodeGen/SelectOptimize.cpp
@@ -1217,7 +1217,7 @@ bool SelectOptimizeImpl::checkLoopHeuristics(const Loop *L,
return true;
OptimizationRemarkMissed ORmissL(DEBUG_TYPE, "SelectOpti",
- L->getHeader()->getFirstNonPHI());
+ &*L->getHeader()->getFirstNonPHIIt());
if (LoopCost[0].NonPredCost > LoopCost[0].PredCost ||
LoopCost[1].NonPredCost >= LoopCost[1].PredCost) {
diff --git a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
index 3e89b18585f153..33c63417444780 100644
--- a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp
@@ -250,7 +250,7 @@ void FunctionLoweringInfo::set(const Function &fn, MachineFunction &mf,
// Don't create MachineBasicBlocks for imaginary EH pad blocks. These blocks
// are really data, and no instructions can live here.
if (BB.isEHPad()) {
- const Instruction *PadInst = BB.getFirstNonPHI();
+ BasicBlock::const_iterator PadInst = BB.getFirstNonPHIIt();
// If this is a non-landingpad EH pad, mark this function as using
// funclets.
// FIXME: SEH catchpads do not create EH scope/funclets, so we could avoid
@@ -261,13 +261,13 @@ void FunctionLoweringInfo::set(const Function &fn, MachineFunction &mf,
MF->getFrameInfo().setHasOpaqueSPAdjustment(true);
}
if (isa<CatchSwitchInst>(PadInst)) {
- assert(&*BB.begin() == PadInst &&
+ assert(BB.begin() == PadInst &&
"WinEHPrepare failed to remove PHIs from imaginary BBs");
continue;
}
if (isa<FuncletPadInst>(PadInst) &&
Personality != EHPersonality::Wasm_CXX)
- assert(&*BB.begin() == PadInst && "WinEHPrepare failed to demote PHIs");
+ assert(BB.begin() == PadInst && "WinEHPrepare failed to demote PHIs");
}
MachineBasicBlock *MBB = mf.CreateMachineBasicBlock(&BB);
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 43a182f9b9c195..09b04cdd954dd1 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -2063,7 +2063,7 @@ static void findWasmUnwindDestinations(
SmallVectorImpl<std::pair<MachineBasicBlock *, BranchProbability>>
&UnwindDests) {
while (EHPadBB) {
- const Instruction *Pad = EHPadBB->getFirstNonPHI();
+ BasicBlock::const_iterator Pad = EHPadBB->getFirstNonPHIIt();
if (isa<CleanupPadInst>(Pad)) {
// Stop on cleanup pads.
UnwindDests.emplace_back(FuncInfo.getMBB(EHPadBB), Prob);
@@ -2111,7 +2111,7 @@ static void findUnwindDestinations(
}
while (EHPadBB) {
- const Instruction *Pad = EHPadBB->getFirstNonPHI();
+ BasicBlock::const_iterator Pad = EHPadBB->getFirstNonPHIIt();
BasicBlock *NewEHPadBB = nullptr;
if (isa<LandingPadInst>(Pad)) {
// Stop on landingpads. They are not funclets.
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
index b416e98fe61a8b..c356f5a0fbb079 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
@@ -1421,7 +1421,7 @@ bool SelectionDAGISel::PrepareEHLandingPad() {
// Catchpads have one live-in register, which typically holds the exception
// pointer or code.
if (isFuncletEHPersonality(Pers)) {
- if (const auto *CPI = dyn_cast<CatchPadInst>(LLVMBB->getFirstNonPHI())) {
+ if (const auto *CPI = dyn_cast<CatchPadInst>(LLVMBB->getFirstNonPHIIt())) {
if (hasExceptionPointerOrCodeUser(CPI)) {
// Get or create the virtual register to hold the pointer or code. Mark
// the live in physreg and copy into the vreg.
@@ -1452,7 +1452,7 @@ bool SelectionDAGISel::PrepareEHLandingPad() {
MF->getRegInfo().addPhysRegsUsedFromRegMask(RegMask);
if (Pers == EHPersonality::Wasm_CXX) {
- if (const auto *CPI = dyn_cast<CatchPadInst>(LLVMBB->getFirstNonPHI()))
+ if (const auto *CPI = dyn_cast<CatchPadInst>(LLVMBB->getFirstNonPHIIt()))
mapWasmLandingPadIndex(MBB, CPI);
} else {
// Assign the call site to the landing pad's begin label.
@@ -1721,13 +1721,12 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
// use anything def'd by or after the tail call.
{
BasicBlock::iterator BBStart =
- const_cast<BasicBlock *>(LLVMBB)->getFirstNonPHI()->getIterator();
+ const_cast<BasicBlock *>(LLVMBB)->getFirstNonPHIIt();
BasicBlock::iterator BBEnd = const_cast<BasicBlock *>(LLVMBB)->end();
preserveFakeUses(BBStart, BBEnd);
}
- BasicBlock::const_iterator const Begin =
- LLVMBB->getFirstNonPHI()->getIterator();
+ BasicBlock::const_iterator const Begin = LLVMBB->getFirstNonPHIIt();
BasicBlock::const_iterator const End = LLVMBB->end();
BasicBlock::const_iterator BI = End;
diff --git a/llvm/lib/CodeGen/WasmEHPrepare.cpp b/llvm/lib/CodeGen/WasmEHPrepare.cpp
index 1701b0d04425d2..d18196b2217f58 100644
--- a/llvm/lib/CodeGen/WasmEHPrepare.cpp
+++ b/llvm/lib/CodeGen/WasmEHPrepare.cpp
@@ -227,7 +227,7 @@ bool WasmEHPrepareImpl::prepareEHPads(Function &F) {
for (BasicBlock &BB : F) {
if (!BB.isEHPad())
continue;
- auto *Pad = BB.getFirstNonPHI();
+ BasicBlock::iterator Pad = BB.getFirstNonPHIIt();
if (isa<CatchPadInst>(Pad))
CatchPads.push_back(&BB);
else if (isa<CleanupPadInst>(Pad))
@@ -284,7 +284,7 @@ bool WasmEHPrepareImpl::prepareEHPads(Function &F) {
unsigned Index = 0;
for (auto *BB : CatchPads) {
- auto *CPI = cast<CatchPadInst>(BB->getFirstNonPHI());
+ auto *CPI = cast<CatchPadInst>(BB->getFirstNonPHIIt());
// In case of a single catch (...), we don't need to emit a personalify
// function call
if (CPI->arg_size() == 1 &&
@@ -309,7 +309,7 @@ void WasmEHPrepareImpl::prepareEHPad(BasicBlock *BB, bool NeedPersonality,
IRBuilder<> IRB(BB->getContext());
IRB.SetInsertPoint(BB, BB->getFirstInsertionPt());
- auto *FPI = cast<FuncletPadInst>(BB->getFirstNonPHI());
+ auto *FPI = cast<FuncletPadInst>(BB->getFirstNonPHIIt());
Instruction *GetExnCI = nullptr, *GetSelectorCI = nullptr;
for (auto &U : FPI->uses()) {
if (auto *CI = dyn_cast<CallInst>(U.getUser())) {
@@ -388,13 +388,13 @@ void llvm::calculateWasmEHInfo(const Function *F, WasmEHFuncInfo &EHInfo) {
for (const auto &BB : *F) {
if (!BB.isEHPad())
continue;
- const Instruction *Pad = BB.getFirstNonPHI();
+ const Instruction *Pad = &*BB.getFirstNonPHIIt();
if (const auto *CatchPad = dyn_cast<CatchPadInst>(Pad)) {
const auto *UnwindBB = CatchPad->getCatchSwitch()->getUnwindDest();
if (!UnwindBB)
continue;
- const Instruction *UnwindPad = UnwindBB->getFirstNonPHI();
+ const Instruction *UnwindPad = &*UnwindBB->getFirstNonPHIIt();
if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(UnwindPad))
// Currently there should be only one handler per a catchswitch.
EHInfo.setUnwindDest(&BB, *CatchSwitch->handlers().begin());
diff --git a/llvm/lib/CodeGen/WinEHPrepare.cpp b/llvm/lib/CodeGen/WinEHPrepare.cpp
index c58c67b70fe3c2..6d85f078290337 100644
--- a/llvm/lib/CodeGen/WinEHPrepare.cpp
+++ b/llvm/lib/CodeGen/WinEHPrepare.cpp
@@ -201,7 +201,7 @@ static void calculateStateNumbersForInvokes(const Function *Fn,
BasicBlock *FuncletUnwindDest;
auto *FuncletPad =
- dyn_cast<FuncletPadInst>(FuncletEntryBB->getFirstNonPHI());
+ dyn_cast<FuncletPadInst>(FuncletEntryBB->getFirstNonPHIIt());
assert(FuncletPad || FuncletEntryBB == &Fn->getEntryBlock());
if (!FuncletPad)
FuncletUnwindDest = nullptr;
@@ -223,7 +223,7 @@ static void calculateStateNumbersForInvokes(const Function *Fn,
if (BaseState != -1) {
FuncInfo.InvokeStateMap[II] = BaseState;
} else {
- Instruction *PadInst = InvokeUnwindDest->getFirstNonPHI();
+ Instruction *PadInst = &*InvokeUnwindDest->getFirstNonPHIIt();
assert(FuncInfo.EHPadStateMap.count(PadInst) && "EH Pad has no state!");
FuncInfo.InvokeStateMap[II] = FuncInfo.EHPadStateMap[PadInst];
}
@@ -254,10 +254,10 @@ void llvm::calculateCXXStateForAsynchEH(const BasicBlock *BB, int State,
if (EHInfo.BlockToStateMap.count(BB) && EHInfo.BlockToStateMap[BB] <= State)
continue; // skip blocks already visited by lower State
- const llvm::Instruction *I = BB->getFirstNonPHI();
+ BasicBlock::const_iterator It = BB->getFirstNonPHIIt();
const llvm::Instruction *TI = BB->getTerminator();
- if (I->isEHPad())
- State = EHInfo.EHPadStateMap[I];
+ if (It->isEHPad())
+ State = EHInfo.EHPadStateMap[&*It];
EHInfo.BlockToStateMap[BB] = State; // Record state, also flag visiting
if ((isa<CleanupReturnInst>(TI) || isa<CatchReturnInst>(TI)) && State > 0) {
@@ -315,15 +315,15 @@ void llvm::calculateSEHStateForAsynchEH(const BasicBlock *BB, int State,
if (EHInfo.BlockToStateMap.count(BB) && EHInfo.BlockToStateMap[BB] <= State)
continue; // skip blocks already visited by lower State
- const llvm::Instruction *I = BB->getFirstNonPHI();
+ BasicBlock::const_iterator It = BB->getFirstNonPHIIt();
const llvm::Instruction *TI = BB->getTerminator();
- if (I->isEHPad())
- State = EHInfo.EHPadStateMap[I];
+ if (It->isEHPad())
+ State = EHInfo.EHPadStateMap[&*It];
EHInfo.BlockToStateMap[BB] = State; // Record state
- if (isa<CatchPadInst>(I) && isa<CatchReturnInst>(TI)) {
+ if (isa<CatchPadInst>(It) && isa<CatchReturnInst>(TI)) {
const Constant *FilterOrNull = cast<Constant>(
- cast<CatchPadInst>(I)->getArgOperand(0)->stripPointerCasts());
+ cast<CatchPadInst>(It)->getArgOperand(0)->stripPointerCasts());
const Function *Filter = dyn_cast<Function>(FilterOrNull);
if (!Filter || !Filter->getName().starts_with("__IsLocalUnwind"))
State = EHInfo.SEHUnwindMap[State].ToState; // Retrive next State
@@ -385,7 +385,7 @@ static void calculateCXXStateNumbers(WinEHFuncInfo &FuncInfo,
SmallVector<const CatchPadInst *, 2> Handlers;
for (const BasicBlock *CatchPadBB : CatchSwitch->handlers()) {
- auto *CatchPad = cast<CatchPadInst>(CatchPadBB->getFirstNonPHI());
+ auto *CatchPad = cast<CatchPadInst>(CatchPadBB->getFirstNonPHIIt());
Handlers.push_back(CatchPad);
}
int TryLow = a...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
SLTozer
approved these changes
Jan 23, 2025
There was a problem hiding this comment.
Code changes LGTM (after formatting). Regarding the direction of the patch, it seems a little unfortunate to completely remove getFirstNonPHI() - sometimes we want the instruction and sometimes we want the iterator, and forcing all of them to go through getFirstNonPHIIt() may increase the chances that the iterator and instruction are viewed as interchangeable and lead to people using &*BB->getFirstNonPHIIt() as an insertion point. I think the chance of this is substantially less likely than using the wrong iterator with the current state of affairs, however!
As part of the "RemoveDIs" project, BasicBlock::iterator now carries a debug-info bit that's needed when getFirstNonPHI and similar feed into instruction insertion positions. Call-sites where that's necessary were updated a year ago; but to ensure some type safety however, we'd like to have all calls to getFirstNonPHI use the iterator-returning version. This patch changes a bunch of call-sites calling getFirstNonPHI to use getFirstNonPHIIt, which returns an iterator. All these call sites are where it's obviously safe to fetch the iterator then dereference it. A follow-up patch will contain less-obviously-safe changes. We'll eventually deprecate and remove the instruction-pointer getFirstNonPHI, but not before adding concise documentation of what considerations are needed (very few).
Co-authored-by: Stephen Tozer <[email protected]>
Indeed, this is the sharp end of RemoveDIs, we can't force all behaviours to be correct by using the type system, we have to accept some scope for misbehaviour. All things considered, I think having the type system bark at you beats relying on people to get it right each time. There's also some scope for static-analysis and dynamic instrumentation if we end up needing to scan for errors / risky behaviours. (Pushing up rebased version then merging) |
jmorse
force-pushed
the
getfirst-non-phi-iterators
branch
from
bfd006b to
222c91b
Compare
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.
(NB: this will fail CI as it composes with #123583, but I'm not gluing them together or it'll be too hard to review).
As part of the "RemoveDIs" project, BasicBlock::iterator now carries a debug-info bit that's needed when getFirstNonPHI and similar feed into instruction insertion positions. Call-sites where that's necessary were updated a year ago; but to ensure some type safety however, we'd like to have all calls to getFirstNonPHI use the iterator-returning version.
This patch changes a bunch of call-sites calling getFirstNonPHI to use getFirstNonPHIIt, which returns an iterator. All these call sites are where it's obviously safe to fetch the iterator then dereference it. A follow-up patch will contain less-obviously-safe changes.
We'll eventually deprecate and remove the instruction-pointer getFirstNonPHI, but not before adding concise documentation of what considerations are needed (very few).