[DebugInfo][RemoveDIs] Have getInsertionPtAfterDef return an iterator #73149
+86
−47
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
Part of the "RemoveDIs" project to remove debug intrinsics requires passing block-positions around in iterators rather than as instruction pointers, allowing some debug-info to reside in BasicBlock::iterator. This means getInsertionPointAfterDef has to return an iterator, and as it can return no-instruction that means returning an optional iterator. This patch changes the signature for getInsertionPtAfterDef and then patches up the various places that use it to handle the different type. This would overall be an NFC patch, however in InstCombinerImpl::freezeOtherUses I've started skipping any debug intrinsics at the returned insert-position. This should not have any _meaningful_ effect on the compiler output: at worst it means variable assignments that are skipped will now cover the freeze instruction and anything inserted before it, which should be inconsequential.
|
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesPart of the "RemoveDIs" project to remove debug intrinsics requires passing block-positions around in iterators rather than as instruction pointers, allowing some debug-info to reside in BasicBlock::iterator. This means getInsertionPointAfterDef has to return an iterator, and as it can return no-instruction that means returning an optional iterator. This patch changes the signature for getInsertionPtAfterDef and then patches up the various places that use it to handle the different type. This would overall be an NFC patch, however in InstCombinerImpl::freezeOtherUses I've started skipping any debug intrinsics at the returned insert-position. This should not have any meaningful effect on the compiler output: at worst it means variable assignments that are skipped will now cover the freeze instruction and anything inserted before it, which should be inconsequential. Sadly: this makes the function signature ugly. This is probably the ugliest piece of fallout for the "RemoveDIs" work, but it serves the overall purpose of improving compile times and not allowing Full diff: https://github.com/llvm/llvm-project/pull/73149.diff 11 Files Affected:
diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h
index e3c4e76f90a4cfc..2cd9a665e9e5d05 100644
--- a/llvm/include/llvm/IR/IRBuilder.h
+++ b/llvm/include/llvm/IR/IRBuilder.h
@@ -200,6 +200,14 @@ class IRBuilderBase {
SetCurrentDebugLocation(IP->getStableDebugLoc());
}
+ /// This specifies that created instructions should be inserted at
+ /// the specified point, but also requires that \p IP is dereferencable.
+ void SetInsertPoint(BasicBlock::iterator IP) {
+ BB = IP->getParent();
+ InsertPt = IP;
+ SetCurrentDebugLocation(IP->getStableDebugLoc());
+ }
+
/// This specifies that created instructions should inserted at the beginning
/// end of the specified function, but after already existing static alloca
/// instructions that are at the start.
diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h
index 58fc32237367d93..48fec7dde468a83 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -224,7 +224,9 @@ class Instruction : public User,
/// of cases, e.g. phi nodes or terminators that return values. This function
/// may return null if the insertion after the definition is not possible,
/// e.g. due to a catchswitch terminator.
- Instruction *getInsertionPointAfterDef();
+ std::optional<
+ SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator>
+ getInsertionPointAfterDef();
//===--------------------------------------------------------------------===//
// Subclass classification.
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 7449692f05d7bf9..e4b3e911a71f19b 100644
--- a/llvm/lib/IR/Instruction.cpp
+++ b/llvm/lib/IR/Instruction.cpp
@@ -274,7 +274,7 @@ bool Instruction::comesBefore(const Instruction *Other) const {
return Order < Other->Order;
}
-Instruction *Instruction::getInsertionPointAfterDef() {
+std::optional<BasicBlock::iterator> Instruction::getInsertionPointAfterDef() {
assert(!getType()->isVoidTy() && "Instruction must define result");
BasicBlock *InsertBB;
BasicBlock::iterator InsertPt;
@@ -287,18 +287,22 @@ Instruction *Instruction::getInsertionPointAfterDef() {
} else if (isa<CallBrInst>(this)) {
// Def is available in multiple successors, there's no single dominating
// insertion point.
- return nullptr;
+ return std::nullopt;
} else {
assert(!isTerminator() && "Only invoke/callbr terminators return value");
InsertBB = getParent();
InsertPt = std::next(getIterator());
+ // Any instruction inserted immediately after "this" will come before any
+ // debug-info records take effect -- thus, set the head bit indicating that
+ // to debug-info-transfer code.
+ InsertPt.setHeadBit(true);
}
// catchswitch blocks don't have any legal insertion point (because they
// are both an exception pad and a terminator).
if (InsertPt == InsertBB->end())
- return nullptr;
- return &*InsertPt;
+ return std::nullopt;
+ return InsertPt;
}
bool Instruction::isOnlyUserOfAnyOperand() {
diff --git a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
index fef1a698e146390..1134b20880f1830 100644
--- a/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroFrame.cpp
@@ -2886,13 +2886,13 @@ void coro::salvageDebugInfo(
// dbg.value since it does not have the same function wide guarantees that
// dbg.declare does.
if (isa<DbgDeclareInst>(DVI)) {
- Instruction *InsertPt = nullptr;
+ std::optional<BasicBlock::iterator> InsertPt;
if (auto *I = dyn_cast<Instruction>(Storage))
InsertPt = I->getInsertionPointAfterDef();
else if (isa<Argument>(Storage))
- InsertPt = &*F->getEntryBlock().begin();
+ InsertPt = F->getEntryBlock().begin();
if (InsertPt)
- DVI->moveBefore(InsertPt);
+ DVI->moveBefore(*(*InsertPt)->getParent(), *InsertPt);
}
}
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
index 02881109f17d29f..557447a352576a6 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
@@ -4130,7 +4130,7 @@ static bool canFreelyInvert(InstCombiner &IC, Value *Op,
static Value *freelyInvert(InstCombinerImpl &IC, Value *Op,
Instruction *IgnoredUser) {
auto *I = cast<Instruction>(Op);
- IC.Builder.SetInsertPoint(&*I->getInsertionPointAfterDef());
+ IC.Builder.SetInsertPoint(&**I->getInsertionPointAfterDef());
Value *NotOp = IC.Builder.CreateNot(Op, Op->getName() + ".not");
Op->replaceUsesWithIf(NotOp,
[NotOp](Use &U) { return U.getUser() != NotOp; });
@@ -4168,7 +4168,7 @@ bool InstCombinerImpl::sinkNotIntoLogicalOp(Instruction &I) {
Op0 = freelyInvert(*this, Op0, &I);
Op1 = freelyInvert(*this, Op1, &I);
- Builder.SetInsertPoint(I.getInsertionPointAfterDef());
+ Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
Value *NewLogicOp;
if (IsBinaryOp)
NewLogicOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
@@ -4216,7 +4216,7 @@ bool InstCombinerImpl::sinkNotIntoOtherHandOfLogicalOp(Instruction &I) {
*OpToInvert = freelyInvert(*this, *OpToInvert, &I);
- Builder.SetInsertPoint(&*I.getInsertionPointAfterDef());
+ Builder.SetInsertPoint(*I.getInsertionPointAfterDef());
Value *NewBinOp;
if (IsBinaryOp)
NewBinOp = Builder.CreateBinOp(NewOpc, Op0, Op1, I.getName() + ".not");
diff --git a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
index f8346cd03849acf..a991f0906052a30 100644
--- a/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
@@ -4017,9 +4017,9 @@ bool InstCombinerImpl::transformConstExprCastCall(CallBase &Call) {
NV = NC = CastInst::CreateBitOrPointerCast(NC, OldRetTy);
NC->setDebugLoc(Caller->getDebugLoc());
- Instruction *InsertPt = NewCall->getInsertionPointAfterDef();
- assert(InsertPt && "No place to insert cast");
- InsertNewInstBefore(NC, InsertPt->getIterator());
+ auto OptInsertPt = NewCall->getInsertionPointAfterDef();
+ assert(OptInsertPt && "No place to insert cast");
+ InsertNewInstBefore(NC, *OptInsertPt);
Worklist.pushUsersToWorkList(*Caller);
} else {
NV = PoisonValue::get(Caller->getType());
diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
index e6088541349784b..597378605125129 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3807,19 +3807,27 @@ bool InstCombinerImpl::freezeOtherUses(FreezeInst &FI) {
// *all* uses if the operand is an invoke/callbr and the use is in a phi on
// the normal/default destination. This is why the domination check in the
// replacement below is still necessary.
- Instruction *MoveBefore;
+ BasicBlock::iterator MoveBefore;
if (isa<Argument>(Op)) {
MoveBefore =
- &*FI.getFunction()->getEntryBlock().getFirstNonPHIOrDbgOrAlloca();
+ FI.getFunction()->getEntryBlock().getFirstNonPHIOrDbgOrAlloca();
} else {
- MoveBefore = cast<Instruction>(Op)->getInsertionPointAfterDef();
- if (!MoveBefore)
+ auto MoveBeforeOpt = cast<Instruction>(Op)->getInsertionPointAfterDef();
+ if (!MoveBeforeOpt)
return false;
+ MoveBefore = *MoveBeforeOpt;
}
+ // Don't move to the position of a debug intrinsic.
+ if (isa<DbgInfoIntrinsic>(MoveBefore))
+ MoveBefore = MoveBefore->getNextNonDebugInstruction()->getIterator();
+ // Re-point iterator to come after any debug-info records, if we're
+ // running in "RemoveDIs" mode
+ MoveBefore.setHeadBit(false);
+
bool Changed = false;
- if (&FI != MoveBefore) {
- FI.moveBefore(*MoveBefore->getParent(), MoveBefore->getIterator());
+ if (&FI != &*MoveBefore) {
+ FI.moveBefore(*MoveBefore->getParent(), MoveBefore);
Changed = true;
}
diff --git a/llvm/lib/Transforms/Scalar/GuardWidening.cpp b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
index 5506dd82efd4557..61d2912ff70fce1 100644
--- a/llvm/lib/Transforms/Scalar/GuardWidening.cpp
+++ b/llvm/lib/Transforms/Scalar/GuardWidening.cpp
@@ -596,35 +596,47 @@ void GuardWideningImpl::makeAvailableAt(Value *V, Instruction *Loc) const {
}
// Return Instruction before which we can insert freeze for the value V as close
-// to def as possible. If there is no place to add freeze, return nullptr.
-static Instruction *getFreezeInsertPt(Value *V, const DominatorTree &DT) {
+// to def as possible. If there is no place to add freeze, return empty.
+static std::optional<BasicBlock::iterator>
+getFreezeInsertPt(Value *V, const DominatorTree &DT) {
auto *I = dyn_cast<Instruction>(V);
if (!I)
- return &*DT.getRoot()->getFirstNonPHIOrDbgOrAlloca();
+ return DT.getRoot()->getFirstNonPHIOrDbgOrAlloca()->getIterator();
- auto *Res = I->getInsertionPointAfterDef();
+ std::optional<BasicBlock::iterator> Res = I->getInsertionPointAfterDef();
// If there is no place to add freeze - return nullptr.
- if (!Res || !DT.dominates(I, Res))
- return nullptr;
+ if (!Res || !DT.dominates(I, &**Res))
+ return std::nullopt;
+
+ Instruction *ResInst = &**Res;
// If there is a User dominated by original I, then it should be dominated
// by Freeze instruction as well.
if (any_of(I->users(), [&](User *U) {
Instruction *User = cast<Instruction>(U);
- return Res != User && DT.dominates(I, User) && !DT.dominates(Res, User);
+ return ResInst != User && DT.dominates(I, User) &&
+ !DT.dominates(ResInst, User);
}))
- return nullptr;
+ return std::nullopt;
return Res;
}
Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) {
if (isGuaranteedNotToBePoison(Orig, nullptr, InsertPt, &DT))
return Orig;
- Instruction *InsertPtAtDef = getFreezeInsertPt(Orig, DT);
- if (!InsertPtAtDef)
- return new FreezeInst(Orig, "gw.freeze", InsertPt);
- if (isa<Constant>(Orig) || isa<GlobalValue>(Orig))
- return new FreezeInst(Orig, "gw.freeze", InsertPtAtDef);
+ std::optional<BasicBlock::iterator> InsertPtAtDef =
+ getFreezeInsertPt(Orig, DT);
+ if (!InsertPtAtDef) {
+ FreezeInst *FI = new FreezeInst(Orig, "gw.freeze");
+ FI->insertBefore(InsertPt);
+ return FI;
+ }
+ if (isa<Constant>(Orig) || isa<GlobalValue>(Orig)) {
+ BasicBlock::iterator InsertPt = *InsertPtAtDef;
+ FreezeInst *FI = new FreezeInst(Orig, "gw.freeze");
+ FI->insertBefore(*InsertPt->getParent(), InsertPt);
+ return FI;
+ }
SmallSet<Value *, 16> Visited;
SmallVector<Value *, 16> Worklist;
@@ -643,8 +655,10 @@ Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) {
if (Visited.insert(Def).second) {
if (isGuaranteedNotToBePoison(Def, nullptr, InsertPt, &DT))
return true;
- CacheOfFreezes[Def] = new FreezeInst(Def, Def->getName() + ".gw.fr",
- getFreezeInsertPt(Def, DT));
+ BasicBlock::iterator InsertPt = *getFreezeInsertPt(Def, DT);
+ FreezeInst *FI = new FreezeInst(Def, Def->getName() + ".gw.fr");
+ FI->insertBefore(*InsertPt->getParent(), InsertPt);
+ CacheOfFreezes[Def] = FI;
}
if (CacheOfFreezes.count(Def))
@@ -685,8 +699,9 @@ Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) {
Value *Result = Orig;
for (Value *V : NeedFreeze) {
- auto *FreezeInsertPt = getFreezeInsertPt(V, DT);
- FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr", FreezeInsertPt);
+ auto FreezeInsertPt = *getFreezeInsertPt(V, DT);
+ FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr");
+ FI->insertBefore(*FreezeInsertPt->getParent(), FreezeInsertPt);
++FreezeAdded;
if (V == Orig)
Result = FI;
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index 4093a5a51a4d79e..095ffda7fff1eaa 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -2411,7 +2411,7 @@ bool LoopIdiomRecognize::recognizeShiftUntilBitTest() {
// it's use count.
Instruction *InsertPt = nullptr;
if (auto *BitPosI = dyn_cast<Instruction>(BitPos))
- InsertPt = BitPosI->getInsertionPointAfterDef();
+ InsertPt = &**BitPosI->getInsertionPointAfterDef();
else
InsertPt = &*DT->getRoot()->getFirstNonPHIOrDbgOrAlloca();
if (!InsertPt)
diff --git a/llvm/lib/Transforms/Scalar/Reassociate.cpp b/llvm/lib/Transforms/Scalar/Reassociate.cpp
index 9c4a344d4295f8a..0d55c72e407e928 100644
--- a/llvm/lib/Transforms/Scalar/Reassociate.cpp
+++ b/llvm/lib/Transforms/Scalar/Reassociate.cpp
@@ -921,16 +921,20 @@ static Value *NegateValue(Value *V, Instruction *BI,
TheNeg->getParent()->getParent() != BI->getParent()->getParent())
continue;
- Instruction *InsertPt;
+ BasicBlock::iterator InsertPt;
if (Instruction *InstInput = dyn_cast<Instruction>(V)) {
- InsertPt = InstInput->getInsertionPointAfterDef();
- if (!InsertPt)
+ auto InsertPtOpt = InstInput->getInsertionPointAfterDef();
+ if (!InsertPtOpt)
continue;
+ InsertPt = *InsertPtOpt;
} else {
- InsertPt = &*TheNeg->getFunction()->getEntryBlock().begin();
+ InsertPt = TheNeg->getFunction()
+ ->getEntryBlock()
+ .getFirstNonPHIOrDbg()
+ ->getIterator();
}
- TheNeg->moveBefore(InsertPt);
+ TheNeg->moveBefore(*InsertPt->getParent(), InsertPt);
if (TheNeg->getOpcode() == Instruction::Sub) {
TheNeg->setHasNoUnsignedWrap(false);
TheNeg->setHasNoSignedWrap(false);
diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index acf2b0e154e8267..f3cf6fbf427bea7 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -1810,16 +1810,16 @@ bool VectorCombine::foldSelectShuffle(Instruction &I, bool FromReduction) {
return SSV->getOperand(Op);
return SV->getOperand(Op);
};
- Builder.SetInsertPoint(SVI0A->getInsertionPointAfterDef());
+ Builder.SetInsertPoint(*SVI0A->getInsertionPointAfterDef());
Value *NSV0A = Builder.CreateShuffleVector(GetShuffleOperand(SVI0A, 0),
GetShuffleOperand(SVI0A, 1), V1A);
- Builder.SetInsertPoint(SVI0B->getInsertionPointAfterDef());
+ Builder.SetInsertPoint(*SVI0B->getInsertionPointAfterDef());
Value *NSV0B = Builder.CreateShuffleVector(GetShuffleOperand(SVI0B, 0),
GetShuffleOperand(SVI0B, 1), V1B);
- Builder.SetInsertPoint(SVI1A->getInsertionPointAfterDef());
+ Builder.SetInsertPoint(*SVI1A->getInsertionPointAfterDef());
Value *NSV1A = Builder.CreateShuffleVector(GetShuffleOperand(SVI1A, 0),
GetShuffleOperand(SVI1A, 1), V2A);
- Builder.SetInsertPoint(SVI1B->getInsertionPointAfterDef());
+ Builder.SetInsertPoint(*SVI1B->getInsertionPointAfterDef());
Value *NSV1B = Builder.CreateShuffleVector(GetShuffleOperand(SVI1B, 0),
GetShuffleOperand(SVI1B, 1), V2B);
Builder.SetInsertPoint(Op0);
|
|
It would be more typical to return end() instead using using std::optional. |
Rather than returning std::nullopt.
|
I've pushed up an equivalent patch using |
Ah yes, that's a very good point. What I had in mind when making the suggestion is the getFirstInsertPt() API, but of course that one is called on a BB and always returns an iterator into that BB. In this case, we might actually return an iterator to a different BB (following the invoke success edge), so that comparing |
This reverts commit 77ff164.
|
No worries; reverted to the initial form. |
OCHyams
approved these changes
Nov 30, 2023
llvm/include/llvm/IR/Instruction.h
Outdated
Comment on lines
227
to
229
| std::optional< | ||
| SymbolTableList<Instruction, ilist_iterator_bits<true>>::iterator> | ||
| getInsertionPointAfterDef(); |
There was a problem hiding this comment.
nit: std::optional<InstListType::iterator>
| @@ -4130,7 +4130,7 @@ static bool canFreelyInvert(InstCombiner &IC, Value *Op, | |||
| static Value *freelyInvert(InstCombinerImpl &IC, Value *Op, | |||
| Instruction *IgnoredUser) { | |||
| auto *I = cast<Instruction>(Op); | |||
| IC.Builder.SetInsertPoint(&*I->getInsertionPointAfterDef()); | |||
| IC.Builder.SetInsertPoint(&**I->getInsertionPointAfterDef()); | |||
There was a problem hiding this comment.
The leading &* are not needed (were not needed before your patch)? And potentially drops haedBit info, if I'm reading this right.
| if (isa<Constant>(Orig) || isa<GlobalValue>(Orig)) { | ||
| BasicBlock::iterator InsertPt = *InsertPtAtDef; | ||
| FreezeInst *FI = new FreezeInst(Orig, "gw.freeze"); | ||
| FI->insertBefore(*InsertPt->getParent(), InsertPt); |
There was a problem hiding this comment.
Should insertBefore be using InsertPtAtDef here instead?
Suggested change
| FI->insertBefore(*InsertPt->getParent(), InsertPt); | |
| FI->insertBefore(*InsertPt->getParent(), *InsertPtAtDef); |
There was a problem hiding this comment.
Could do -- the motivation of unwrapping into a local iterator variable was to avoid a double-deref for the first argument to insertBefore -- it always makes me feel like there's something wrong, so I figured the unwrapping would make it more legible + understandable. (YMMV).
| @@ -685,8 +699,9 @@ Value *GuardWideningImpl::freezeAndPush(Value *Orig, Instruction *InsertPt) { | |||
|
|
|||
| Value *Result = Orig; | |||
| for (Value *V : NeedFreeze) { | |||
| auto *FreezeInsertPt = getFreezeInsertPt(V, DT); | |||
| FreezeInst *FI = new FreezeInst(V, V->getName() + ".gw.fr", FreezeInsertPt); | |||
| auto FreezeInsertPt = *getFreezeInsertPt(V, DT); | |||
There was a problem hiding this comment.
nit: used auto here but BasicBlock::iterator above
| BasicBlock::iterator InsertPt = *getFreezeInsertPt(Def, DT); | ||
| FreezeInst *FI = new FreezeInst(Def, Def->getName() + ".gw.fr"); | ||
| FI->insertBefore(*InsertPt->getParent(), InsertPt); | ||
| CacheOfFreezes[Def] = FI; |
There was a problem hiding this comment.
It's a shame about the added verbosity. Is there anything we can do about that? (Update/add instruction ctors as we go along, etc).
There was a problem hiding this comment.
We can certainly restore this back to "insert in the constructor", I'm just shying away from landing that right now. IMO it's better to land as one-foul-swoop which breaks downstream builds, but only does it once.
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.
Part of the "RemoveDIs" project to remove debug intrinsics requires passing block-positions around in iterators rather than as instruction pointers, allowing some debug-info to reside in BasicBlock::iterator. This means getInsertionPointAfterDef has to return an iterator, and as it can return no-instruction that means returning an optional iterator.
This patch changes the signature for getInsertionPtAfterDef and then patches up the various places that use it to handle the different type. This would overall be an NFC patch, however in InstCombinerImpl::freezeOtherUses I've started skipping any debug intrinsics at the returned insert-position. This should not have any meaningful effect on the compiler output: at worst it means variable assignments that are skipped will now cover the freeze instruction and anything inserted before it, which should be inconsequential.
Sadly: this makes the function signature ugly. This is probably the ugliest piece of fallout for the "RemoveDIs" work, but it serves the overall purpose of improving compile times and not allowing
-gto affect compiler output, so should be worthwhile in the end.