[llvm] 2425e29 - [DebugInfo][RemoveDIs] Have getInsertionPtAfterDef return an iterator (#73149)
via llvm-commits
llvm-commits at lists.llvm.org
Thu Nov 30 04:20:03 PST 2023
Author: Jeremy Morse
Date: 2023-11-30T12:19:57Z
New Revision: 2425e2940eb4f24de296ada92e25d6aad9578cd0
URL: https://github.com/llvm/llvm-project/commit/2425e2940eb4f24de296ada92e25d6aad9578cd0
DIFF: https://github.com/llvm/llvm-project/commit/2425e2940eb4f24de296ada92e25d6aad9578cd0.diff
LOG: [DebugInfo][RemoveDIs] Have getInsertionPtAfterDef return an iterator (#73149)
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 `-g` to
affect compiler output, so should be worthwhile in the end.
Added:
Modified:
llvm/include/llvm/IR/IRBuilder.h
llvm/include/llvm/IR/Instruction.h
llvm/lib/IR/Instruction.cpp
llvm/lib/Transforms/Coroutines/CoroFrame.cpp
llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp
llvm/lib/Transforms/InstCombine/InstCombineCalls.cpp
llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
llvm/lib/Transforms/Scalar/GuardWidening.cpp
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
llvm/lib/Transforms/Scalar/Reassociate.cpp
llvm/lib/Transforms/Vectorize/VectorCombine.cpp
Removed:
################################################################################
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..a676e1d4dea49d8 100644
--- a/llvm/include/llvm/IR/Instruction.h
+++ b/llvm/include/llvm/IR/Instruction.h
@@ -224,7 +224,7 @@ 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<InstListType::iterator> getInsertionPointAfterDef();
//===--------------------------------------------------------------------===//
// Subclass classification.
diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp
index 97797e99371d600..b6a422477a672c6 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..1ca6085e36de3b7 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 146d6b6ca3f6e17..fa39f9bf8718bf6 100644
--- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -3823,19 +3823,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..3bbf6642a90cf7c 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);
+ BasicBlock::iterator 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 13e88204c7b47f4..3721564890ddb4e 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 f386c855acc51a4..f18711ba30b7088 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);
More information about the llvm-commits
mailing list