Skip to content

Commit 01ac6d1

Browse files
zmodemzmodem
committed
Revert "[DebugInfo] Handle multiple variable location operands in IR"
This caused non-deterministic compiler output; see comment on the code review. > This patch updates the various IR passes to correctly handle dbg.values with a > DIArgList location. This patch does not actually allow DIArgLists to be produced > by salvageDebugInfo, and it does not affect any pass after codegen-prepare. > Other than that, it should cover every IR pass. > > Most of the changes simply extend code that operated on a single debug value to > operate on the list of debug values in the style of any_of, all_of, for_each, > etc. Instances of setOperand(0, ...) have been replaced with with > replaceVariableLocationOp, which takes the value that is being replaced as an > additional argument. In places where this value isn't readily available, we have > to track the old value through to the point where it gets replaced. > > Differential Revision: https://reviews.llvm.org/D88232 This reverts commit df69c69.
1 parent c165a99 commit 01ac6d1

File tree

19 files changed

+147
-284
lines changed

19 files changed

+147
-284
lines changed

‎llvm/include/llvm/IR/IntrinsicInst.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,6 @@ class DbgVariableIntrinsic : public DbgInfoIntrinsic {
203203
Value *getVariableLocationOp(unsigned OpIdx) const;
204204

205205
void replaceVariableLocationOp(Value *OldValue, Value *NewValue);
206-
void replaceVariableLocationOp(unsigned OpIdx, Value *NewValue);
207206

208207
void setVariable(DILocalVariable *NewVar) {
209208
setArgOperand(1, MetadataAsValue::get(NewVar->getContext(), NewVar));

‎llvm/include/llvm/Transforms/Utils/Local.h

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -311,10 +311,9 @@ void salvageDebugInfoForDbgValues(Instruction &I,
311311
/// Given an instruction \p I and DIExpression \p DIExpr operating on it, write
312312
/// the effects of \p I into the returned DIExpression, or return nullptr if
313313
/// it cannot be salvaged. \p StackVal: whether DW_OP_stack_value should be
314-
/// appended to the expression. \p LocNo: the index of the location operand to
315-
/// which \p I applies, should be 0 for debug info without a DIArgList.
314+
/// appended to the expression.
316315
DIExpression *salvageDebugInfoImpl(Instruction &I, DIExpression *DIExpr,
317-
bool StackVal, unsigned LocNo);
316+
bool StackVal);
318317

319318
/// Point debug users of \p From to \p To or salvage them. Use this function
320319
/// only when replacing all uses of \p From with \p To, with a guarantee that

‎llvm/lib/CodeGen/CodeGenPrepare.cpp

Lines changed: 34 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -2873,16 +2873,11 @@ class TypePromotionTransaction {
28732873
/// Keep track of the debug users.
28742874
SmallVector<DbgValueInst *, 1> DbgValues;
28752875

2876-
/// Keep track of the new value so that we can undo it by replacing
2877-
/// instances of the new value with the original value.
2878-
Value *New;
2879-
28802876
using use_iterator = SmallVectorImpl<InstructionAndIdx>::iterator;
28812877

28822878
public:
28832879
/// Replace all the use of \p Inst by \p New.
2884-
UsesReplacer(Instruction *Inst, Value *New)
2885-
: TypePromotionAction(Inst), New(New) {
2880+
UsesReplacer(Instruction *Inst, Value *New) : TypePromotionAction(Inst) {
28862881
LLVM_DEBUG(dbgs() << "Do: UsersReplacer: " << *Inst << " with " << *New
28872882
<< "\n");
28882883
// Record the original uses.
@@ -2908,7 +2903,7 @@ class TypePromotionTransaction {
29082903
// the original debug uses must also be reinstated to maintain the
29092904
// correctness and utility of debug value instructions.
29102905
for (auto *DVI : DbgValues)
2911-
DVI->replaceVariableLocationOp(New, Inst);
2906+
DVI->replaceVariableLocationOp(DVI->getVariableLocationOp(0), Inst);
29122907
}
29132908
};
29142909

@@ -7908,21 +7903,18 @@ bool CodeGenPrepare::fixupDbgValue(Instruction *I) {
79087903
DbgValueInst &DVI = *cast<DbgValueInst>(I);
79097904

79107905
// Does this dbg.value refer to a sunk address calculation?
7911-
bool AnyChange = false;
7912-
for (Value *Location : DVI.getValues()) {
7913-
WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
7914-
Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
7915-
if (SunkAddr) {
7916-
// Point dbg.value at locally computed address, which should give the best
7917-
// opportunity to be accurately lowered. This update may change the type
7918-
// of pointer being referred to; however this makes no difference to
7919-
// debugging information, and we can't generate bitcasts that may affect
7920-
// codegen.
7921-
DVI.replaceVariableLocationOp(Location, SunkAddr);
7922-
AnyChange = true;
7923-
}
7924-
}
7925-
return AnyChange;
7906+
Value *Location = DVI.getVariableLocationOp(0);
7907+
WeakTrackingVH SunkAddrVH = SunkAddrs[Location];
7908+
Value *SunkAddr = SunkAddrVH.pointsToAliveValue() ? SunkAddrVH : nullptr;
7909+
if (SunkAddr) {
7910+
// Point dbg.value at locally computed address, which should give the best
7911+
// opportunity to be accurately lowered. This update may change the type of
7912+
// pointer being referred to; however this makes no difference to debugging
7913+
// information, and we can't generate bitcasts that may affect codegen.
7914+
DVI.replaceVariableLocationOp(Location, SunkAddr);
7915+
return true;
7916+
}
7917+
return false;
79267918
}
79277919

79287920
// A llvm.dbg.value may be using a value before its definition, due to
@@ -7941,51 +7933,30 @@ bool CodeGenPrepare::placeDbgValues(Function &F) {
79417933
if (!DVI)
79427934
continue;
79437935

7944-
SmallVector<Instruction *, 4> VIs;
7945-
for (Value *V : DVI->getValues())
7946-
if (Instruction *VI = dyn_cast_or_null<Instruction>(V))
7947-
VIs.push_back(VI);
7948-
7949-
// This DVI may depend on multiple instructions, complicating any
7950-
// potential sink. This block takes the defensive approach, opting to
7951-
// "undef" the DVI if it has more than one instruction and any of them do
7952-
// not dominate DVI.
7953-
for (Instruction *VI : VIs) {
7954-
if (VI->isTerminator())
7955-
continue;
7936+
Instruction *VI = dyn_cast_or_null<Instruction>(DVI->getValue());
79567937

7957-
// If VI is a phi in a block with an EHPad terminator, we can't insert
7958-
// after it.
7959-
if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
7960-
continue;
7938+
if (!VI || VI->isTerminator())
7939+
continue;
79617940

7962-
// If the defining instruction dominates the dbg.value, we do not need
7963-
// to move the dbg.value.
7964-
if (DT.dominates(VI, DVI))
7965-
continue;
7941+
// If VI is a phi in a block with an EHPad terminator, we can't insert
7942+
// after it.
7943+
if (isa<PHINode>(VI) && VI->getParent()->getTerminator()->isEHPad())
7944+
continue;
79667945

7967-
// If we depend on multiple instructions and any of them doesn't
7968-
// dominate this DVI, we probably can't salvage it: moving it to
7969-
// after any of the instructions could cause us to lose the others.
7970-
if (VIs.size() > 1) {
7971-
LLVM_DEBUG(
7972-
dbgs()
7973-
<< "Unable to find valid location for Debug Value, undefing:\n"
7974-
<< *DVI);
7975-
DVI->setUndef();
7976-
break;
7977-
}
7946+
// If the defining instruction dominates the dbg.value, we do not need
7947+
// to move the dbg.value.
7948+
if (DT.dominates(VI, DVI))
7949+
continue;
79787950

7979-
LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
7980-
<< *DVI << ' ' << *VI);
7981-
DVI->removeFromParent();
7982-
if (isa<PHINode>(VI))
7983-
DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
7984-
else
7985-
DVI->insertAfter(VI);
7986-
MadeChange = true;
7987-
++NumDbgValueMoved;
7988-
}
7951+
LLVM_DEBUG(dbgs() << "Moving Debug Value before :\n"
7952+
<< *DVI << ' ' << *VI);
7953+
DVI->removeFromParent();
7954+
if (isa<PHINode>(VI))
7955+
DVI->insertBefore(&*VI->getParent()->getFirstInsertionPt());
7956+
else
7957+
DVI->insertAfter(VI);
7958+
MadeChange = true;
7959+
++NumDbgValueMoved;
79897960
}
79907961
}
79917962
return MadeChange;

‎llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1260,8 +1260,7 @@ void SelectionDAGBuilder::salvageUnresolvedDbgValue(DanglingDebugInfo &DDI) {
12601260
// variable. FIXME: Further work could recover those too.
12611261
while (isa<Instruction>(V)) {
12621262
Instruction &VAsInst = *cast<Instruction>(V);
1263-
// Temporary "0", awaiting real implementation.
1264-
DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue, 0);
1263+
DIExpression *NewExpr = salvageDebugInfoImpl(VAsInst, Expr, StackValue);
12651264

12661265
// If we cannot salvage any further, and haven't yet found a suitable debug
12671266
// expression, bail out.
@@ -6054,7 +6053,7 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
60546053
DILocalVariable *Variable = DI.getVariable();
60556054
DIExpression *Expression = DI.getExpression();
60566055
dropDanglingDebugInfo(Variable, Expression);
6057-
SmallVector<Value *, 4> Values(DI.getValues());
6056+
SmallVector<Value *> Values(DI.getValues());
60586057
if (Values.empty())
60596058
return;
60606059

‎llvm/lib/IR/IntrinsicInst.cpp

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -98,24 +98,6 @@ void DbgVariableIntrinsic::replaceVariableLocationOp(Value *OldValue,
9898
setArgOperand(
9999
0, MetadataAsValue::get(getContext(), DIArgList::get(getContext(), MDs)));
100100
}
101-
void DbgVariableIntrinsic::replaceVariableLocationOp(unsigned OpIdx,
102-
Value *NewValue) {
103-
assert(OpIdx < getNumVariableLocationOps() && "Invalid Operand Index");
104-
if (!hasArgList()) {
105-
Value *NewOperand = isa<MetadataAsValue>(NewValue)
106-
? NewValue
107-
: MetadataAsValue::get(
108-
getContext(), ValueAsMetadata::get(NewValue));
109-
return setArgOperand(0, NewOperand);
110-
}
111-
SmallVector<ValueAsMetadata *, 4> MDs;
112-
ValueAsMetadata *NewOperand = getAsMetadata(NewValue);
113-
for (unsigned Idx = 0; Idx < getNumVariableLocationOps(); ++Idx)
114-
MDs.push_back(Idx == OpIdx ? NewOperand
115-
: getAsMetadata(getVariableLocationOp(Idx)));
116-
setArgOperand(
117-
0, MetadataAsValue::get(getContext(), DIArgList::get(getContext(), MDs)));
118-
}
119101

120102
Optional<uint64_t> DbgVariableIntrinsic::getFragmentSizeInBits() const {
121103
if (auto Fragment = getExpression()->getFragmentInfo())

‎llvm/lib/IR/User.cpp

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,6 @@ void User::replaceUsesOfWith(Value *From, Value *To) {
3131
// most importantly, removing "this" from the use list of "From".
3232
setOperand(i, To);
3333
}
34-
if (auto DVI = dyn_cast_or_null<DbgVariableIntrinsic>(this)) {
35-
if (is_contained(DVI->location_ops(), From))
36-
DVI->replaceVariableLocationOp(From, To);
37-
}
3834
}
3935

4036
//===----------------------------------------------------------------------===//

‎llvm/lib/Target/AArch64/AArch64StackTagging.cpp

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -563,9 +563,10 @@ bool AArch64StackTagging::runOnFunction(Function &Fn) {
563563
}
564564

565565
if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(I)) {
566-
for (Value *V : DVI->location_ops())
567-
if (auto *AI = dyn_cast_or_null<AllocaInst>(V))
568-
Allocas[AI].DbgVariableIntrinsics.push_back(DVI);
566+
if (auto *AI =
567+
dyn_cast_or_null<AllocaInst>(DVI->getVariableLocationOp(0))) {
568+
Allocas[AI].DbgVariableIntrinsics.push_back(DVI);
569+
}
569570
continue;
570571
}
571572

‎llvm/lib/Transforms/Coroutines/CoroFrame.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2173,7 +2173,7 @@ void coro::salvageDebugInfo(
21732173
Storage = StInst->getOperand(0);
21742174
} else if (auto *GEPInst = dyn_cast<GetElementPtrInst>(Storage)) {
21752175
Expr = llvm::salvageDebugInfoImpl(*GEPInst, Expr,
2176-
/*WithStackValue=*/false, 0);
2176+
/*WithStackValue=*/false);
21772177
if (!Expr)
21782178
return;
21792179
Storage = GEPInst->getOperand(0);

‎llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3586,14 +3586,23 @@ static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
35863586
llvm::sort(DbgUsersToSink,
35873587
[](auto *A, auto *B) { return B->comesBefore(A); });
35883588

3589+
// Update the arguments of a dbg.declare instruction, so that it
3590+
// does not point into a sunk instruction.
3591+
auto updateDbgDeclare = [](DbgVariableIntrinsic *DII) {
3592+
if (!isa<DbgDeclareInst>(DII))
3593+
return false;
3594+
3595+
return true;
3596+
};
3597+
35893598
SmallVector<DbgVariableIntrinsic *, 2> DIIClones;
35903599
SmallSet<DebugVariable, 4> SunkVariables;
35913600
for (auto User : DbgUsersToSink) {
35923601
// A dbg.declare instruction should not be cloned, since there can only be
35933602
// one per variable fragment. It should be left in the original place
35943603
// because the sunk instruction is not an alloca (otherwise we could not be
35953604
// here).
3596-
if (isa<DbgDeclareInst>(User))
3605+
if (updateDbgDeclare(User))
35973606
continue;
35983607

35993608
DebugVariable DbgUserVariable =
@@ -3604,8 +3613,6 @@ static bool TryToSinkInstruction(Instruction *I, BasicBlock *DestBlock) {
36043613
continue;
36053614

36063615
DIIClones.emplace_back(cast<DbgVariableIntrinsic>(User->clone()));
3607-
if (isa<DbgDeclareInst>(User) && isa<CastInst>(I))
3608-
DIIClones.back()->replaceVariableLocationOp(I, I->getOperand(0));
36093616
LLVM_DEBUG(dbgs() << "CLONE: " << *DIIClones.back() << '\n');
36103617
}
36113618

‎llvm/lib/Transforms/Instrumentation/HWAddressSanitizer.cpp

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1218,10 +1218,10 @@ bool HWAddressSanitizer::sanitizeFunction(Function &F) {
12181218
isa<CleanupReturnInst>(Inst))
12191219
RetVec.push_back(&Inst);
12201220

1221-
if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst))
1222-
for (Value *V : DVI->location_ops())
1223-
if (auto *Alloca = dyn_cast_or_null<AllocaInst>(V))
1224-
AllocaDbgMap[Alloca].push_back(DVI);
1221+
if (auto *DDI = dyn_cast<DbgVariableIntrinsic>(&Inst))
1222+
if (auto *Alloca =
1223+
dyn_cast_or_null<AllocaInst>(DDI->getVariableLocationOp(0)))
1224+
AllocaDbgMap[Alloca].push_back(DDI);
12251225

12261226
if (InstrumentLandingPads && isa<LandingPadInst>(Inst))
12271227
LandingPadVec.push_back(&Inst);
@@ -1297,18 +1297,13 @@ bool HWAddressSanitizer::sanitizeFunction(Function &F) {
12971297
}
12981298

12991299
if (!AllocaToPaddedAllocaMap.empty()) {
1300-
for (auto &BB : F) {
1301-
for (auto &Inst : BB) {
1302-
if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst)) {
1303-
for (Value *V : DVI->location_ops()) {
1304-
if (auto *AI = dyn_cast_or_null<AllocaInst>(V)) {
1305-
if (auto *NewAI = AllocaToPaddedAllocaMap.lookup(AI))
1306-
DVI->replaceVariableLocationOp(V, NewAI);
1307-
}
1308-
}
1309-
}
1310-
}
1311-
}
1300+
for (auto &BB : F)
1301+
for (auto &Inst : BB)
1302+
if (auto *DVI = dyn_cast<DbgVariableIntrinsic>(&Inst))
1303+
if (auto *AI =
1304+
dyn_cast_or_null<AllocaInst>(DVI->getVariableLocationOp(0)))
1305+
if (auto *NewAI = AllocaToPaddedAllocaMap.lookup(AI))
1306+
DVI->replaceVariableLocationOp(AI, NewAI);
13121307
for (auto &P : AllocaToPaddedAllocaMap)
13131308
P.first->eraseFromParent();
13141309
}

‎llvm/lib/Transforms/Scalar/ADCE.cpp

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -521,14 +521,10 @@ bool AggressiveDeadCodeElimination::removeDeadInstructions() {
521521
// If intrinsic is pointing at a live SSA value, there may be an
522522
// earlier optimization bug: if we know the location of the variable,
523523
// why isn't the scope of the location alive?
524-
for (Value *V : DII->location_ops()) {
525-
if (Instruction *II = dyn_cast<Instruction>(V)) {
526-
if (isLive(II)) {
524+
if (Value *V = DII->getVariableLocationOp(0))
525+
if (Instruction *II = dyn_cast<Instruction>(V))
526+
if (isLive(II))
527527
dbgs() << "Dropping debug info for " << *DII << "\n";
528-
break;
529-
}
530-
}
531-
}
532528
}
533529
}
534530
});

0 commit comments

Comments
 (0)