[DebugInfo][RemoveDIs] Support maintaining DPValues in CodeGenPrepare #73660
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
CodeGenPrepare needs to support the maintenence of DPValues, the non-instruction replacement for dbg.value intrinsics. This means there are a few functions we need to duplicate or replicate the functionality of: * fixupDbgValue for setting users of sunk addr GEPs, * The remains of placeDbgValues needs a DPValue implementation for sinking * Rollback of RAUWs needs to update DPValues * Rollback of instruction removal needs supporting (see github llvm#73350) * A few places where we have to use iterators rather than instructions. There are three places where we have to use the setHeadBit call on iterators to indicate which portion of debug-info records we're about to splice around. This is because CodeGenPrepare, unlike other optimisation passes, is very much concerned with which block an operation occurs in and where in the block instructions are because it's preparing things to be in a format that's good for SelectionDAG. There isn't a large amount of test coverage for debuginfo behaviours in this pass, hence I've added some more.
|
@llvm/pr-subscribers-llvm-ir @llvm/pr-subscribers-debuginfo Author: Jeremy Morse (jmorse) ChangesCodeGenPrepare needs to support the maintenence of DPValues, the non-instruction replacement for dbg.value intrinsics. This means there are a few functions we need to duplicate or replicate the functionality of:
There are three places where we have to use the setHeadBit call on iterators to indicate which portion of debug-info records we're about to splice around. This is because CodeGenPrepare, unlike other optimisation passes, is very much concerned with which block an operation occurs in and where in the block instructions are because it's preparing things to be in a format that's good for SelectionDAG. There isn't a large amount of test coverage for debuginfo behaviours in this pass, hence I've added some more. Patch is 21.80 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/73660.diff 7 Files Affected:
diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 07dc718ee3a38c3..0ea01087e46875f 100644
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -459,7 +459,9 @@ class CodeGenPrepare : public FunctionPass {
bool optimizeExtractElementInst(Instruction *Inst);
bool dupRetToEnableTailCallOpts(BasicBlock *BB, ModifyDT &ModifiedDT);
bool fixupDbgValue(Instruction *I);
+ bool fixupDPValue(DPValue &I);
bool placeDbgValues(Function &F);
+ bool placeDPValues(Instruction &I, DominatorTree &DT);
bool placePseudoProbes(Function &F);
bool canFormExtLd(const SmallVectorImpl<Instruction *> &MovedExts,
LoadInst *&LI, Instruction *&Inst, bool HasPromoted);
@@ -1753,8 +1755,8 @@ static bool sinkCmpExpression(CmpInst *Cmp, const TargetLowering &TLI) {
BasicBlock::iterator InsertPt = UserBB->getFirstInsertionPt();
assert(InsertPt != UserBB->end());
InsertedCmp = CmpInst::Create(Cmp->getOpcode(), Cmp->getPredicate(),
- Cmp->getOperand(0), Cmp->getOperand(1), "",
- &*InsertPt);
+ Cmp->getOperand(0), Cmp->getOperand(1), "");
+ InsertedCmp->insertBefore(*UserBB, InsertPt);
// Propagate the debug info.
InsertedCmp->setDebugLoc(Cmp->getDebugLoc());
}
@@ -2066,10 +2068,13 @@ SinkShiftAndTruncate(BinaryOperator *ShiftI, Instruction *User, ConstantInt *CI,
// Sink the trunc
BasicBlock::iterator TruncInsertPt = TruncUserBB->getFirstInsertionPt();
TruncInsertPt++;
+ // It will go ahead of any debug-info.
+ TruncInsertPt.setHeadBit(true); // not covered by any test at all.
assert(TruncInsertPt != TruncUserBB->end());
InsertedTrunc = CastInst::Create(TruncI->getOpcode(), InsertedShift,
- TruncI->getType(), "", &*TruncInsertPt);
+ TruncI->getType(), "");
+ InsertedTrunc->insertBefore(*TruncUserBB, TruncInsertPt);
InsertedTrunc->setDebugLoc(TruncI->getDebugLoc());
MadeChange = true;
@@ -2234,7 +2239,9 @@ static bool despeculateCountZeros(IntrinsicInst *CountZeros,
// Create another block after the count zero intrinsic. A PHI will be added
// in this block to select the result of the intrinsic or the bit-width
// constant if the input to the intrinsic is zero.
- BasicBlock::iterator SplitPt = ++(BasicBlock::iterator(CountZeros));
+ BasicBlock::iterator SplitPt = std::next(BasicBlock::iterator(CountZeros));
+ // Any debug-info after CountZeros should not be included.
+ SplitPt.setHeadBit(true); // coveredy by Transforms/CodeGenPrepare/X86/cttz-ctlz.ll
BasicBlock *EndBlock = CallBlock->splitBasicBlock(SplitPt, "cond.end");
if (IsHugeFunc)
FreshBBs.insert(EndBlock);
@@ -2829,6 +2836,7 @@ class TypePromotionTransaction {
Instruction *PrevInst;
BasicBlock *BB;
} Point;
+ std::optional<DPValue::self_iterator> beforeDPValue = std::nullopt;
/// Remember whether or not the instruction had a previous instruction.
bool HasPrevInstruction;
@@ -2836,12 +2844,21 @@ class TypePromotionTransaction {
public:
/// Record the position of \p Inst.
InsertionHandler(Instruction *Inst) {
- BasicBlock::iterator It = Inst->getIterator();
- HasPrevInstruction = (It != (Inst->getParent()->begin()));
- if (HasPrevInstruction)
- Point.PrevInst = &*--It;
- else
- Point.BB = Inst->getParent();
+ HasPrevInstruction = (Inst != &*(Inst->getParent()->begin()));
+ BasicBlock *BB = Inst->getParent();
+
+ // Record where we would have to re-insert the instruction in the sequence
+ // of DPValues, if we ended up reinserting.
+ if (BB->IsNewDbgInfoFormat) {
+ DPMarker *DPM = BB->createMarker(Inst);
+ beforeDPValue = DPM->getReinsertionPosition();
+ }
+
+ if (HasPrevInstruction) {
+ Point.PrevInst = &*std::prev(Inst->getIterator());
+ } else {
+ Point.BB = BB;
+ }
}
/// Insert \p Inst at the recorded position.
@@ -2849,14 +2866,16 @@ class TypePromotionTransaction {
if (HasPrevInstruction) {
if (Inst->getParent())
Inst->removeFromParent();
- Inst->insertAfter(Point.PrevInst);
+ Inst->insertAfter(&*Point.PrevInst);
} else {
- Instruction *Position = &*Point.BB->getFirstInsertionPt();
+ BasicBlock::iterator Position = Point.BB->getFirstInsertionPt();
if (Inst->getParent())
- Inst->moveBefore(Position);
+ Inst->moveBefore(*Point.BB, Position);
else
- Inst->insertBefore(Position);
+ Inst->insertBefore(*Point.BB, Position);
}
+
+ Inst->getParent()->reinsertInstInDPValues(Inst, beforeDPValue);
}
};
@@ -2882,6 +2901,7 @@ class TypePromotionTransaction {
};
/// Set the operand of an instruction with a new value.
+ // XXX jmorse -- what about for dbg.values?
class OperandSetter : public TypePromotionAction {
/// Original operand of the instruction.
Value *Origin;
@@ -3059,6 +3079,8 @@ class TypePromotionTransaction {
SmallVector<InstructionAndIdx, 4> OriginalUses;
/// Keep track of the debug users.
SmallVector<DbgValueInst *, 1> DbgValues;
+ /// And non-instruction debug-users too.
+ SmallVector<DPValue *, 1> DPValues;
/// Keep track of the new value so that we can undo it by replacing
/// instances of the new value with the original value.
@@ -3079,7 +3101,7 @@ class TypePromotionTransaction {
}
// Record the debug uses separately. They are not in the instruction's
// use list, but they are replaced by RAUW.
- findDbgValues(DbgValues, Inst);
+ findDbgValues(DbgValues, Inst, &DPValues);
// Now, we can replace the uses.
Inst->replaceAllUsesWith(New);
@@ -3096,6 +3118,10 @@ class TypePromotionTransaction {
// correctness and utility of debug value instructions.
for (auto *DVI : DbgValues)
DVI->replaceVariableLocationOp(New, Inst);
+ // Similar story with DPValues, the non-instruction representation of
+ // dbg.values.
+ for (DPValue *DPV : DPValues) // tested by transaction-test I'm adding
+ DPV->replaceVariableLocationOp(New, Inst);
}
};
@@ -6749,7 +6775,7 @@ bool CodeGenPrepare::optimizeLoadExt(LoadInst *Load) {
!TLI->isLoadExtLegal(ISD::ZEXTLOAD, LoadResultVT, TruncVT))
return false;
- IRBuilder<> Builder(Load->getNextNode());
+ IRBuilder<> Builder(Load->getNextNonDebugInstruction());
auto *NewAnd = cast<Instruction>(
Builder.CreateAnd(Load, ConstantInt::get(Ctx, DemandBits)));
// Mark this instruction as "inserted by CGP", so that other
@@ -7005,7 +7031,9 @@ bool CodeGenPrepare::optimizeSelectInst(SelectInst *SI) {
// Split the select block, according to how many (if any) values go on each
// side.
BasicBlock *StartBlock = SI->getParent();
- BasicBlock::iterator SplitPt = ++(BasicBlock::iterator(LastSI));
+ BasicBlock::iterator SplitPt = std::next(BasicBlock::iterator(LastSI));
+ // We should split before any debug-info.
+ SplitPt.setHeadBit(true); // covered by X86/select.ll
IRBuilder<> IB(SI);
auto *CondFr = IB.CreateFreeze(SI->getCondition(), SI->getName() + ".frozen");
@@ -7017,18 +7045,18 @@ bool CodeGenPrepare::optimizeSelectInst(SelectInst *SI) {
BranchInst *FalseBranch = nullptr;
if (TrueInstrs.size() == 0) {
FalseBranch = cast<BranchInst>(SplitBlockAndInsertIfElse(
- CondFr, &*SplitPt, false, nullptr, nullptr, LI));
+ CondFr, SplitPt, false, nullptr, nullptr, LI));
FalseBlock = FalseBranch->getParent();
EndBlock = cast<BasicBlock>(FalseBranch->getOperand(0));
} else if (FalseInstrs.size() == 0) {
TrueBranch = cast<BranchInst>(SplitBlockAndInsertIfThen(
- CondFr, &*SplitPt, false, nullptr, nullptr, LI));
+ CondFr, SplitPt, false, nullptr, nullptr, LI));
TrueBlock = TrueBranch->getParent();
EndBlock = cast<BasicBlock>(TrueBranch->getOperand(0));
} else {
Instruction *ThenTerm = nullptr;
Instruction *ElseTerm = nullptr;
- SplitBlockAndInsertIfThenElse(CondFr, &*SplitPt, &ThenTerm, &ElseTerm,
+ SplitBlockAndInsertIfThenElse(CondFr, SplitPt, &ThenTerm, &ElseTerm,
nullptr, nullptr, LI);
TrueBranch = cast<BranchInst>(ThenTerm);
FalseBranch = cast<BranchInst>(ElseTerm);
@@ -8095,10 +8123,14 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI,
}
bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
+ bool AnyChange = false;
+ for (DPValue &DPV : I->getDbgValueRange())
+ AnyChange |= fixupDPValue(DPV);
+
// Bail out if we inserted the instruction to prevent optimizations from
// stepping on each other's toes.
if (InsertedInsts.count(I))
- return false;
+ return AnyChange;
// TODO: Move into the switch on opcode below here.
if (PHINode *P = dyn_cast<PHINode>(I)) {
@@ -8112,7 +8144,7 @@ bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
++NumPHIsElim;
return true;
}
- return false;
+ return AnyChange;
}
if (CastInst *CI = dyn_cast<CastInst>(I)) {
@@ -8123,7 +8155,7 @@ bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
// the address of globals out of a loop). If this is the case, we don't
// want to forward-subst the cast.
if (isa<Constant>(CI->getOperand(0)))
- return false;
+ return AnyChange;
if (OptimizeNoopCopyExpression(CI, *TLI, *DL))
return true;
@@ -8149,7 +8181,7 @@ bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
return MadeChange | optimizeExtUses(I);
}
}
- return false;
+ return AnyChange;
}
if (auto *Cmp = dyn_cast<CmpInst>(I))
@@ -8244,7 +8276,7 @@ bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
return true;
}
}
- return false;
+ return AnyChange;
}
if (tryToSinkFreeOperands(I))
@@ -8269,7 +8301,7 @@ bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) {
return optimizeBranch(cast<BranchInst>(I), *TLI, FreshBBs, IsHugeFunc);
}
- return false;
+ return AnyChange;
}
/// Given an OR instruction, check to see if this is a bitreverse
@@ -8359,6 +8391,30 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
return AnyChange;
}
+bool CodeGenPrepare::fixupDPValue(DPValue &DPV) {
+ if (DPV.Type != DPValue::LocationType::Value)
+ return false;
+
+ // Does this DPValue refer to a sunk address calculation?
+ bool AnyChange = false;
+ SmallDenseSet<Value *> LocationOps(DPV.location_ops().begin(),
+ DPV.location_ops().end());
+ for (Value *Location : LocationOps) {
+ WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
+ Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
+ if (SunkAddr) {
+ // Point dbg.value at locally computed address, which should give the best
+ // opportunity to be accurately lowered. This update may change the type
+ // of pointer being referred to; however this makes no difference to
+ // debugging information, and we can't generate bitcasts that may affect
+ // codegen.
+ DPV.replaceVariableLocationOp(Location, SunkAddr);
+ AnyChange = true;
+ }
+ }
+ return AnyChange;
+}
+
// A llvm.dbg.value may be using a value before its definition, due to
// optimizations in this pass and others. Scan for such dbg.values, and rescue
// them by moving the dbg.value to immediately after the value definition.
@@ -8371,8 +8427,10 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {
for (BasicBlock &BB : F) {
for (Instruction &Insn : llvm::make_early_inc_range(BB)) {
DbgValueInst *DVI = dyn_cast<DbgValueInst>(&Insn);
- if (!DVI)
+ if (!DVI) {
+ MadeChange |= placeDPValues(Insn, DT);
continue;
+ }
SmallVector<Instruction *, 4> VIs;
for (Value *V : DVI->getValues())
@@ -8421,6 +8479,65 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {
}
}
}
+
+ return MadeChange;
+}
+
+bool CodeGenPrepare::placeDPValues(Instruction &I, DominatorTree &DT) {
+ bool MadeChange = false;
+
+ for (DPValue &DPV : llvm::make_early_inc_range(I.getDbgValueRange())) {
+ if (DPV.Type != DPValue::LocationType::Value)
+ continue;
+
+ SmallVector<Instruction *, 4> VIs;
+ for (Value *V : DPV.location_ops())
+ if (Instruction *VI = dyn_cast_or_null<Instruction>(V))
+ VIs.push_back(VI);
+
+ // This DPV may depend on multiple instructions, complicating any
+ // potential sink. This block takes the defensive approach, opting to
+ // "undef" the DVI if it has more than one instruction and any of them do
+ // not dominate DVI.
+ for (Instruction *VI : VIs) {
+ if (VI->isTerminator())
+ continue;
+
+ // If VI is a phi in a block with an EHPad terminator, we can't insert
+ // after it.
+ if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
+ continue;
+
+ // If the defining instruction dominates the position, we do not need
+ // to move the dbg.value.
+ if (DT.dominates(VI, &I))
+ continue;
+
+ // If we depend on multiple instructions and any of them doesn't
+ // dominate this DPV, we probably can't salvage it: moving it to
+ // after any of the instructions could cause us to lose the others.
+ if (VIs.size() > 1) {
+ LLVM_DEBUG(
+ dbgs()
+ << "Unable to find valid location for Debug Value, undefing:\n"
+ << DPV);
+ DPV.setKillLocation();
+ break;
+ }
+
+ LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
+ << DPV << ' ' << *VI);
+ DPV.removeFromParent();
+ BasicBlock *VIBB = VI->getParent();
+ if (isa<PHINode>(VI)) {
+ VIBB->insertDPValueBefore(&DPV, VIBB->getFirstInsertionPt());
+ } else {
+ VIBB->insertDPValueAfter(&DPV, VI);
+ }
+ MadeChange = true;
+ ++NumDbgValueMoved;
+ }
+ }
return MadeChange;
}
diff --git a/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll b/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
index 794efccca575bae..3fe4b085525afa0 100644
--- a/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
+++ b/llvm/test/DebugInfo/X86/codegenprep-addrsink.ll
@@ -1,4 +1,5 @@
; RUN: llc -start-before=codegenprepare -stop-after=codegenprepare -mtriple=x86_64-unknown-unknown %s -o - | FileCheck %s
+; RUN: llc -start-before=codegenprepare -stop-after=codegenprepare -mtriple=x86_64-unknown-unknown %s -o - --try-experimental-debuginfo-iterators | FileCheck %s
;
; CGP duplicates address calculation into each basic block that contains loads
; or stores, so that they can be folded into instruction memory operands for
diff --git a/llvm/test/DebugInfo/X86/codegenprepare-rollback.ll b/llvm/test/DebugInfo/X86/codegenprepare-rollback.ll
new file mode 100644
index 000000000000000..e0cb22fbd57661f
--- /dev/null
+++ b/llvm/test/DebugInfo/X86/codegenprepare-rollback.ll
@@ -0,0 +1,32 @@
+; RUN: opt -S -debugify -codegenprepare %s -o - | FileCheck %s --check-prefix=DEBUGIFY
+; RUN: opt -S -debugify -codegenprepare %s -o - --try-experimental-debuginfo-iterators | FileCheck %s --check-prefix=DEBUGIFY
+;
+; Copied from codegen-prepare-addrmode-sext.ll -- for the twoArgsNoPromotion
+; function, CGP attempts a type promotion transaction on the sext to replace
+; it with %add, but then rolls it back. This involves re-inserting the sext
+; instruction between two dbg.value intrinsics, and un-RAUWing the users of
+; the sext.
+; This test checks that this works correctly in both dbg.value mode, but also
+; RemoveDIs non-intrinsic debug-info mode.
+
+target datalayout = "e-i64:64-f80:128-s:64-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx"
+
+; DEBUGIFY-LABEL: @twoArgsNoPromotion
+; DEBUGIFY-NEXT: %add = add
+; DEBUGIFY-NEXT: call void @llvm.dbg.value(metadata i32 %add,
+; DEBUGIFY-NEXT: %sextadd = sext
+; DEBUGIFY-NEXT: call void @llvm.dbg.value(metadata i64 %sextadd,
+; DEBUGIFY-NEXT: %arrayidx = getelementptr
+; DEBUGIFY-NEXT: call void @llvm.dbg.value(metadata ptr %arrayidx,
+; DEBUGIFY-NEXT: %res = load i8,
+; DEBUGIFY-NEXT: call void @llvm.dbg.value(metadata i8 %res,
+; DEBUGIFY-NEXT: ret i8 %res,
+define i8 @twoArgsNoPromotion(i32 %arg1, i32 %arg2, ptr %base) {
+ %add = add nsw i32 %arg1, %arg2
+ %sextadd = sext i32 %add to i64
+ %arrayidx = getelementptr inbounds i8, ptr %base, i64 %sextadd
+ %res = load i8, ptr %arrayidx
+ ret i8 %res
+}
+
diff --git a/llvm/test/Transforms/CodeGenPrepare/X86/catchpad-phi-cast.ll b/llvm/test/Transforms/CodeGenPrepare/X86/catchpad-phi-cast.ll
index d23dd8c792e0e64..614e80e3e89c22f 100644
--- a/llvm/test/Transforms/CodeGenPrepare/X86/catchpad-phi-cast.ll
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/catchpad-phi-cast.ll
@@ -1,4 +1,5 @@
; RUN: opt -codegenprepare -S < %s | FileCheck %s
+; RUN: opt -codegenprepare -S < %s --try-experimental-debuginfo-iterators | FileCheck %s
; The following target lines are needed for the test to exercise what it should.
; Without these lines, CodeGenPrepare does not try to sink the bitcasts.
diff --git a/llvm/test/Transforms/CodeGenPrepare/X86/cttz-ctlz.ll b/llvm/test/Transforms/CodeGenPrepare/X86/cttz-ctlz.ll
index 440afdeff10a3e8..5f368faf46ee31e 100644
--- a/llvm/test/Transforms/CodeGenPrepare/X86/cttz-ctlz.ll
+++ b/llvm/test/Transforms/CodeGenPrepare/X86/cttz-ctlz.ll
@@ -3,6 +3,9 @@
; RUN: opt -S -codegenprepare -mattr=+bmi < %s | FileCheck %s --check-prefix=FAST_TZ
; RUN: opt -S -codegenprepare -mattr=+lzcnt < %s | FileCheck %s --check-prefix=FAST_LZ
+; RUN: opt -S -debugify -codegenprepare < %s | FileCheck %s --check-prefix=DEBUGINFO
+; RUN: opt -S -debugify -codegenprepare --try-experimental-debuginfo-iterators < %s | FileCheck %s --check-prefix=DEBUGINFO
+
target triple = "x86_64-unknown-unknown"
target datalayout = "e-n32:64"
@@ -40,6 +43,19 @@ define i64 @cttz(i64 %A) {
; FAST_LZ-NEXT: [[CTZ:%.*]] = phi i64 [ 64, [[ENTRY:%.*]] ], [ [[Z]], [[COND_FALSE]] ]
; FAST_LZ-NEXT: ret i64 [[CTZ]]
;
+; DEBUGINFO-LABEL: @cttz(
+; DEBUGINFO-NEXT: entry:
+; DEBUGINFO-NEXT: [[A_FR:%.*]] = freeze i64 [[A:%.*]], !dbg [[DBG11:![0-9]+]]
+; DEBUGINFO-NEXT: [[CMPZ:%.*]] = icmp eq i64 [[A_FR]], 0, !dbg [[DBG11]]
+; DEBUGINFO-NEXT: br i1 [[CMPZ]], label [[COND_END:%.*]], label [[COND_FALSE:%.*]], !dbg [[DBG11]]
+; DEBUGINFO: cond.false:
+; DEBUGINFO-NEXT: [[Z:%.*]] = call i64 @llvm.cttz.i64(i64 [[A_FR]], i1 true), !dbg [[DBG11]]
+; DEBUGINFO-NEXT: br label [[COND_END]], !dbg [[DBG12:![0-9]+]]
+; DEBUGINFO: cond.end:
+; DEBUGINFO-NEXT: [[CTZ:%.*]] = phi i64 [ 64, [[ENTRY:%.*]] ], [ [[Z]], [[COND_FALSE]] ], !dbg [[DBG12]]
+; DEBUGINFO-NEXT: tail call void @llvm.dbg.value(metadata i64 [[CTZ]], metadata [[META9:![0-9]+]], metadata !DIExpression()), !dbg [[DBG11]]
+; DEBUGINFO-NEXT: ret i64 [[CTZ]], !dbg [[DBG12]]
+;
entry:
%z = call i64 @llvm.cttz.i64(i64 %A, i1 false)
ret i64 %z
@@ -75,6 +91,19 @@ define i64 @ctlz(i64 %A) {
; FAST_LZ-NEXT: [[Z:%.*]] = call i64 @llvm.ctlz.i64(i64 [[A:%.*]], i1 false)
; FAST_LZ-NEXT: ret i64 [[Z]]
;
+; DEBUGINFO-LABEL: @ctlz(
+; DEBUGINFO-NEXT: entry:
+; DEBUGINFO-NEXT: [[A_FR:%.*]] = freeze i64 [[A:%.*]], !dbg [[DBG16:![0-9]+]]
+; DEBUGINFO-NEXT: [[CMPZ:%.*]] = icmp eq i64 [[A_FR]], 0, !dbg [[DBG16]]
+; DEBUGINFO-NEXT: br i1 [[CMPZ]], label [[COND_END:%.*]], label [[COND_FALSE:%.*]], !dbg [[DBG16]]
+; DEBUGINFO: cond.false:
+; DEBUGINFO-NEXT: [[Z:%.*]] = call i64 @llvm.ctlz.i64(i64 [[A_FR]], i1 true), !dbg [[DBG16]]
+; DEBUGINFO-NEXT: br label [[COND_END]], !dbg [[DBG17:![0-9]+]]
+; DEBUGINFO: cond.end:
+; DEBUGINFO-NEXT: [[CTZ:%.*]] = phi i64 [ 64, [[ENTRY:%.*]] ], [ [[Z]], [[COND_FALSE]] ], !dbg [[DBG17]]
+; DEBUGINFO-NEXT: tail call void @llvm.dbg.value(metadata i64 [[CTZ]], metadata [[META15:![0-9]+]], metadata !DIExpression()), !dbg [[DBG16]]
+; DEBUGINFO-NEXT: ret i64 [[CTZ]], !dbg [[DBG17]]
+;
entry:
%z = call i64 @llvm.ct...
[truncated]
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
Added a bonus-patch that uses This is covered by the test |
OCHyams
reviewed
Nov 29, 2023
llvm/lib/CodeGen/CodeGenPrepare.cpp
Outdated
| BasicBlock::iterator SplitPt = ++(BasicBlock::iterator(CountZeros)); | ||
| BasicBlock::iterator SplitPt = std::next(BasicBlock::iterator(CountZeros)); | ||
| // Any debug-info after CountZeros should not be included. | ||
| SplitPt.setHeadBit(true); // coveredy by Transforms/CodeGenPrepare/X86/cttz-ctlz.ll |
There was a problem hiding this comment.
Did you intend to leave the test coverage comments in? There's are a few others scattered about too.
llvm/lib/CodeGen/CodeGenPrepare.cpp
Outdated
| @@ -2829,34 +2836,46 @@ class TypePromotionTransaction { | |||
| Instruction *PrevInst; | |||
| BasicBlock *BB; | |||
| } Point; | |||
| std::optional<DPValue::self_iterator> beforeDPValue = std::nullopt; | |||
There was a problem hiding this comment.
nit: beforeDPValue -> BeforeDPValue
llvm/lib/CodeGen/CodeGenPrepare.cpp
Outdated
| // of DPValues, if we ended up reinserting. | ||
| if (BB->IsNewDbgInfoFormat) { | ||
| DPMarker *DPM = BB->createMarker(Inst); | ||
| beforeDPValue = DPM->getReinsertionPosition(); |
There was a problem hiding this comment.
Looking at the implementation of DPMarker::getReinsertionPosition, it looks like creating a marker just for this call is possibly a waste - I don't think any Marker-specific state is used in getReinsertPosition? I might be reading it wrong -- either way, I'll drop a comment on the other review.
There was a problem hiding this comment.
(Will remangle with the other patch too)
llvm/lib/CodeGen/CodeGenPrepare.cpp
Outdated
| @@ -2882,6 +2901,7 @@ class TypePromotionTransaction { | |||
| }; | |||
|
|
|||
| /// Set the operand of an instruction with a new value. | |||
| // XXX jmorse -- what about for dbg.values? | |||
| @@ -6749,7 +6775,7 @@ bool CodeGenPrepare::optimizeLoadExt(LoadInst *Load) { | |||
| !TLI->isLoadExtLegal(ISD::ZEXTLOAD, LoadResultVT, TruncVT)) | |||
| return false; | |||
|
|
|||
| IRBuilder<> Builder(Load->getNextNode()); | |||
| IRBuilder<> Builder(Load->getNextNonDebugInstruction()); | |||
| @@ -8095,10 +8123,14 @@ static bool optimizeBranch(BranchInst *Branch, const TargetLowering &TLI, | |||
| } | |||
|
|
|||
| bool CodeGenPrepare::optimizeInst(Instruction *I, ModifyDT &ModifiedDT) { | |||
| bool AnyChange = false; | |||
| for (DPValue &DPV : I->getDbgValueRange()) | |||
| AnyChange |= fixupDPValue(DPV); | |||
There was a problem hiding this comment.
I suppose we want fixupDPValues to affect the return value "has a made change" to maintain parity with dbg.values. But is that desirable behaviour overall? Is this something we want to come back and change in the future? If so, wdyt of adding add a FIXME comment.
| @@ -8359,6 +8391,30 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) { | |||
| return AnyChange; | |||
| } | |||
|
|
|||
| bool CodeGenPrepare::fixupDPValue(DPValue &DPV) { | |||
There was a problem hiding this comment.
The debug intrinsic path asserts that it's a DbgValueInst. fixupDbgValue is only called on dbg_value (and dbg_assign)-type intrinsics, whereas fixupDPVAlue is called without first filtering by DPValue kind. This is fine - no action required.
llvm/lib/CodeGen/CodeGenPrepare.cpp
Outdated
| @@ -8371,8 +8427,10 @@ bool CodeGenPrepare::placeDbgValues(Function &F) { | |||
| for (BasicBlock &BB : F) { | |||
| for (Instruction &Insn : llvm::make_early_inc_range(BB)) { | |||
| DbgValueInst *DVI = dyn_cast<DbgValueInst>(&Insn); | |||
| if (!DVI) | |||
| if (!DVI) { | |||
| MadeChange |= placeDPValues(Insn, DT); | |||
There was a problem hiding this comment.
Shouldn't placeDPValues be placed before that if (!DVI) check, because a DbgValueInst can have DPValues attached. It's possibly an error if we have both DP- and dbg-values, so maybe it doesn't matter.
There was a problem hiding this comment.
Good question -- IMO DPValues on a DbgValueInst isn't a case we should consider as it indicates that something has gone horribly wrong already (and I think there's an assertion in the basic-block converter to catch that?). Were it a DbgDeclare that'd be different as we're supporting those in parallel right now.
| MadeChange = true; | ||
| ++NumDbgValueMoved; | ||
| } | ||
| } |
There was a problem hiding this comment.
Can this be folded back into the placeDbgValues function using the "turn the loop into a lambda and then call for_each(DbgValues, Lambda), for_each(DPValues, Lambda)" idiom (workshopping the name still).
Here's a problem for the RemoveDIs project to make debug-info not be stored
in instructions -- in the following sequence:
dbg.value(foo
%bar = add i32 ...
dbg.value(baz
It's possible for rare passes (only CodeGenPrepare) to remove the add
instruction, and then re-insert it back in the same place. When debug-info
is stored in instructions and there's a total order on "when" things happen
this is easy, but by moving that information out of the instruction stream
we start having to do manual maintenance.
This patch adds some utilities for re-inserting an instruction into a
sequence of DPValue objects. Someday we hope to design this away, but for
now it's necessary to support all the things you can do with dbg.values.
The two unit tests show how DPValues get shuffled around using the relevant
function calls. A follow-up patch adds instrumentation to CodeGenPrepare.
Fix tests for insertAfter faff
I can't believe that after deleting it 4 years ago I'm still working on this function X_X
OCHyams
approved these changes
Nov 29, 2023
jmorse
added a commit
that referenced
this pull request
Nov 30, 2023
…#73660) CodeGenPrepare needs to support the maintenence of DPValues, the non-instruction replacement for dbg.value intrinsics. This means there are a few functions we need to duplicate or replicate the functionality of: * fixupDbgValue for setting users of sunk addr GEPs, * The remains of placeDbgValues needs a DPValue implementation for sinking * Rollback of RAUWs needs to update DPValues * Rollback of instruction removal needs supporting (see github #73350) * A few places where we have to use iterators rather than instructions. There are three places where we have to use the setHeadBit call on iterators to indicate which portion of debug-info records we're about to splice around. This is because CodeGenPrepare, unlike other optimisation passes, is very much concerned with which block an operation occurs in and where in the block instructions are because it's preparing things to be in a format that's good for SelectionDAG. There isn't a large amount of test coverage for debuginfo behaviours in this pass, hence I've added some more.
|
Seeing how the commits pushed here got to be a mess, merged in 3ef98bc |
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.
CodeGenPrepare needs to support the maintenence of DPValues, the non-instruction replacement for dbg.value intrinsics. This means there are a few functions we need to duplicate or replicate the functionality of:
There are three places where we have to use the setHeadBit call on iterators to indicate which portion of debug-info records we're about to splice around. This is because CodeGenPrepare, unlike other optimisation passes, is very much concerned with which block an operation occurs in and where in the block instructions are because it's preparing things to be in a format that's good for SelectionDAG.
There isn't a large amount of test coverage for debuginfo behaviours in this pass, hence I've added some more.