From 76197ea6f91f802467f2614e1217e99eb4037200 Mon Sep 17 00:00:00 2001 From: Jeremy Morse Date: Wed, 11 Jun 2025 14:51:13 +0100 Subject: [PATCH] Revert "[DebugInfo][RemoveDIs] Remove some debug intrinsic-only codepaths (#143451)" This reverts commit c71a2e688828ab3ede4fb54168a674ff68396f61. /me squints -- this is hitting an assertion I thought had been deleted, will revert and investigate for a bit. --- llvm/lib/CodeGen/SelectionDAG/FastISel.cpp | 3 + llvm/lib/IR/AutoUpgrade.cpp | 25 ++++-- llvm/lib/IR/DIBuilder.cpp | 97 +++++++++++++++++----- llvm/lib/IR/DebugInfo.cpp | 19 ++++- llvm/lib/Transforms/Utils/LoopUtils.cpp | 36 +++++--- llvm/unittests/IR/IRBuilderTest.cpp | 10 +++ 6 files changed, 150 insertions(+), 40 deletions(-) diff --git a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp index e8a3df3366b2..59cd0dc8dd34 100644 --- a/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/FastISel.cpp @@ -1671,6 +1671,9 @@ void FastISel::fastEmitBranch(MachineBasicBlock *MSucc, const DebugLoc &DbgLoc) { const BasicBlock *BB = FuncInfo.MBB->getBasicBlock(); bool BlockHasMultipleInstrs = &BB->front() != &BB->back(); + // Handle legacy case of debug intrinsics + if (BlockHasMultipleInstrs && !BB->getModule()->IsNewDbgInfoFormat) + BlockHasMultipleInstrs = BB->sizeWithoutDebug() > 1; if (BlockHasMultipleInstrs && FuncInfo.MBB->isLayoutSuccessor(MSucc)) { // For more accurate line information if this is the only non-debug // instruction in the block then emit it, otherwise we have the diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp index a0886776ff93..cb90af36f3d9 100644 --- a/llvm/lib/IR/AutoUpgrade.cpp +++ b/llvm/lib/IR/AutoUpgrade.cpp @@ -4490,6 +4490,7 @@ void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) { Builder.SetInsertPoint(CI->getParent(), CI->getIterator()); if (!NewFn) { + bool FallthroughToDefaultUpgrade = false; // Get the Function's name. StringRef Name = F->getName(); @@ -4517,15 +4518,29 @@ void llvm::UpgradeIntrinsicCall(CallBase *CI, Function *NewFn) { } else if (IsAMDGCN) { Rep = upgradeAMDGCNIntrinsicCall(Name, CI, F, Builder); } else if (IsDbg) { - upgradeDbgIntrinsicToDbgRecord(Name, CI); + // We might have decided we don't want the new format after all between + // first requesting the upgrade and now; skip the conversion if that is + // the case, and check here to see if the intrinsic needs to be upgraded + // normally. + if (!CI->getModule()->IsNewDbgInfoFormat) { + bool NeedsUpgrade = + upgradeIntrinsicFunction1(CI->getCalledFunction(), NewFn, false); + if (!NeedsUpgrade) + return; + FallthroughToDefaultUpgrade = true; + } else { + upgradeDbgIntrinsicToDbgRecord(Name, CI); + } } else { llvm_unreachable("Unknown function for CallBase upgrade."); } - if (Rep) - CI->replaceAllUsesWith(Rep); - CI->eraseFromParent(); - return; + if (!FallthroughToDefaultUpgrade) { + if (Rep) + CI->replaceAllUsesWith(Rep); + CI->eraseFromParent(); + return; + } } const auto &DefaultCase = [&]() -> void { diff --git a/llvm/lib/IR/DIBuilder.cpp b/llvm/lib/IR/DIBuilder.cpp index 1484c549dd58..5e5ff22132e9 100644 --- a/llvm/lib/IR/DIBuilder.cpp +++ b/llvm/lib/IR/DIBuilder.cpp @@ -1047,13 +1047,36 @@ DbgInstPtr DIBuilder::insertDbgAssign(Instruction *LinkedInstr, Value *Val, LinkedInstr->getMetadata(LLVMContext::MD_DIAssignID)); assert(Link && "Linked instruction must have DIAssign metadata attached"); - DbgVariableRecord *DVR = DbgVariableRecord::createDVRAssign( - Val, SrcVar, ValExpr, Link, Addr, AddrExpr, DL); - // Insert after LinkedInstr. - BasicBlock::iterator NextIt = std::next(LinkedInstr->getIterator()); - NextIt.setHeadBit(true); - insertDbgVariableRecord(DVR, NextIt); - return DVR; + if (M.IsNewDbgInfoFormat) { + DbgVariableRecord *DVR = DbgVariableRecord::createDVRAssign( + Val, SrcVar, ValExpr, Link, Addr, AddrExpr, DL); + // Insert after LinkedInstr. + BasicBlock::iterator NextIt = std::next(LinkedInstr->getIterator()); + NextIt.setHeadBit(true); + insertDbgVariableRecord(DVR, NextIt); + return DVR; + } + + LLVMContext &Ctx = LinkedInstr->getContext(); + Module *M = LinkedInstr->getModule(); + if (!AssignFn) + AssignFn = Intrinsic::getOrInsertDeclaration(M, Intrinsic::dbg_assign); + + std::array Args = { + MetadataAsValue::get(Ctx, ValueAsMetadata::get(Val)), + MetadataAsValue::get(Ctx, SrcVar), + MetadataAsValue::get(Ctx, ValExpr), + MetadataAsValue::get(Ctx, Link), + MetadataAsValue::get(Ctx, ValueAsMetadata::get(Addr)), + MetadataAsValue::get(Ctx, AddrExpr), + }; + + IRBuilder<> B(Ctx); + B.SetCurrentDebugLocation(DL); + + auto *DVI = cast(B.CreateCall(AssignFn, Args)); + DVI->insertAfter(LinkedInstr->getIterator()); + return DVI; } /// Initialize IRBuilder for inserting dbg.declare and dbg.value intrinsics. @@ -1078,10 +1101,18 @@ DbgInstPtr DIBuilder::insertDbgValueIntrinsic(llvm::Value *Val, DIExpression *Expr, const DILocation *DL, InsertPosition InsertPt) { - DbgVariableRecord *DVR = - DbgVariableRecord::createDbgVariableRecord(Val, VarInfo, Expr, DL); - insertDbgVariableRecord(DVR, InsertPt); - return DVR; + if (M.IsNewDbgInfoFormat) { + DbgVariableRecord *DVR = + DbgVariableRecord::createDbgVariableRecord(Val, VarInfo, Expr, DL); + insertDbgVariableRecord(DVR, InsertPt); + return DVR; + } + + if (!ValueFn) + ValueFn = Intrinsic::getOrInsertDeclaration(&M, Intrinsic::dbg_value); + auto *DVI = insertDbgIntrinsic(ValueFn, Val, VarInfo, Expr, DL, InsertPt); + cast(DVI)->setTailCall(); + return DVI; } DbgInstPtr DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo, @@ -1093,10 +1124,25 @@ DbgInstPtr DIBuilder::insertDeclare(Value *Storage, DILocalVariable *VarInfo, VarInfo->getScope()->getSubprogram() && "Expected matching subprograms"); - DbgVariableRecord *DVR = - DbgVariableRecord::createDVRDeclare(Storage, VarInfo, Expr, DL); - insertDbgVariableRecord(DVR, InsertPt); - return DVR; + if (M.IsNewDbgInfoFormat) { + DbgVariableRecord *DVR = + DbgVariableRecord::createDVRDeclare(Storage, VarInfo, Expr, DL); + insertDbgVariableRecord(DVR, InsertPt); + return DVR; + } + + if (!DeclareFn) + DeclareFn = getDeclareIntrin(M); + + trackIfUnresolved(VarInfo); + trackIfUnresolved(Expr); + Value *Args[] = {getDbgIntrinsicValueImpl(VMContext, Storage), + MetadataAsValue::get(VMContext, VarInfo), + MetadataAsValue::get(VMContext, Expr)}; + + IRBuilder<> B(DL->getContext()); + initIRBuilder(B, DL, InsertPt); + return B.CreateCall(DeclareFn, Args); } void DIBuilder::insertDbgVariableRecord(DbgVariableRecord *DVR, @@ -1145,12 +1191,23 @@ DbgInstPtr DIBuilder::insertLabel(DILabel *LabelInfo, const DILocation *DL, "Expected matching subprograms"); trackIfUnresolved(LabelInfo); - DbgLabelRecord *DLR = new DbgLabelRecord(LabelInfo, DL); - if (InsertPt.isValid()) { - auto *BB = InsertPt.getBasicBlock(); - BB->insertDbgRecordBefore(DLR, InsertPt); + if (M.IsNewDbgInfoFormat) { + DbgLabelRecord *DLR = new DbgLabelRecord(LabelInfo, DL); + if (InsertPt.isValid()) { + auto *BB = InsertPt.getBasicBlock(); + BB->insertDbgRecordBefore(DLR, InsertPt); + } + return DLR; } - return DLR; + + if (!LabelFn) + LabelFn = Intrinsic::getOrInsertDeclaration(&M, Intrinsic::dbg_label); + + Value *Args[] = {MetadataAsValue::get(VMContext, LabelInfo)}; + + IRBuilder<> B(DL->getContext()); + initIRBuilder(B, DL, InsertPt); + return B.CreateCall(LabelFn, Args); } void DIBuilder::replaceVTableHolder(DICompositeType *&T, DIType *VTableHolder) { diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp index 2a84e7bae0f1..7db9891fdbd7 100644 --- a/llvm/lib/IR/DebugInfo.cpp +++ b/llvm/lib/IR/DebugInfo.cpp @@ -2123,11 +2123,22 @@ static void emitDbgAssign(AssignmentInfo Info, Value *Val, Value *Dest, Expr = *R; } DIExpression *AddrExpr = DIExpression::get(StoreLikeInst.getContext(), {}); - auto *Assign = DbgVariableRecord::createLinkedDVRAssign( - &StoreLikeInst, Val, VarRec.Var, Expr, Dest, AddrExpr, VarRec.DL); + if (StoreLikeInst.getParent()->IsNewDbgInfoFormat) { + auto *Assign = DbgVariableRecord::createLinkedDVRAssign( + &StoreLikeInst, Val, VarRec.Var, Expr, Dest, AddrExpr, VarRec.DL); + (void)Assign; + LLVM_DEBUG(if (Assign) errs() << " > INSERT: " << *Assign << "\n"); + return; + } + auto Assign = DIB.insertDbgAssign(&StoreLikeInst, Val, VarRec.Var, Expr, Dest, + AddrExpr, VarRec.DL); (void)Assign; - LLVM_DEBUG(if (Assign) errs() << " > INSERT: " << *Assign << "\n"); - return; + LLVM_DEBUG(if (!Assign.isNull()) { + if (const auto *Record = dyn_cast(Assign)) + errs() << " > INSERT: " << *Record << "\n"; + else + errs() << " > INSERT: " << *cast(Assign) << "\n"; + }); } #undef DEBUG_TYPE // Silence redefinition warning (from ConstantsContext.h). diff --git a/llvm/lib/Transforms/Utils/LoopUtils.cpp b/llvm/lib/Transforms/Utils/LoopUtils.cpp index ff69fa9f70c4..0681ebc111cb 100644 --- a/llvm/lib/Transforms/Utils/LoopUtils.cpp +++ b/llvm/lib/Transforms/Utils/LoopUtils.cpp @@ -606,6 +606,7 @@ void llvm::deleteDeadLoop(Loop *L, DominatorTree *DT, ScalarEvolution *SE, // Use a map to unique and a vector to guarantee deterministic ordering. llvm::SmallDenseSet DeadDebugSet; + llvm::SmallVector DeadDebugInst; llvm::SmallVector DeadDbgVariableRecords; if (ExitBlock) { @@ -632,19 +633,29 @@ void llvm::deleteDeadLoop(Loop *L, DominatorTree *DT, ScalarEvolution *SE, U.set(Poison); } - // For one of each variable encountered, preserve a debug record (set + // RemoveDIs: do the same as below for DbgVariableRecords. + if (Block->IsNewDbgInfoFormat) { + for (DbgVariableRecord &DVR : llvm::make_early_inc_range( + filterDbgVars(I.getDbgRecordRange()))) { + DebugVariable Key(DVR.getVariable(), DVR.getExpression(), + DVR.getDebugLoc().get()); + if (!DeadDebugSet.insert(Key).second) + continue; + // Unlinks the DVR from it's container, for later insertion. + DVR.removeFromParent(); + DeadDbgVariableRecords.push_back(&DVR); + } + } + + // For one of each variable encountered, preserve a debug intrinsic (set // to Poison) and transfer it to the loop exit. This terminates any // variable locations that were set during the loop. - for (DbgVariableRecord &DVR : - llvm::make_early_inc_range(filterDbgVars(I.getDbgRecordRange()))) { - DebugVariable Key(DVR.getVariable(), DVR.getExpression(), - DVR.getDebugLoc().get()); - if (!DeadDebugSet.insert(Key).second) - continue; - // Unlinks the DVR from it's container, for later insertion. - DVR.removeFromParent(); - DeadDbgVariableRecords.push_back(&DVR); - } + auto *DVI = dyn_cast(&I); + if (!DVI) + continue; + if (!DeadDebugSet.insert(DebugVariable(DVI)).second) + continue; + DeadDebugInst.push_back(DVI); } // After the loop has been deleted all the values defined and modified @@ -660,6 +671,9 @@ void llvm::deleteDeadLoop(Loop *L, DominatorTree *DT, ScalarEvolution *SE, "There should be a non-PHI instruction in exit block, else these " "instructions will have no parent."); + for (auto *DVI : DeadDebugInst) + DVI->moveBefore(*ExitBlock, InsertDbgValueBefore); + // Due to the "head" bit in BasicBlock::iterator, we're going to insert // each DbgVariableRecord right at the start of the block, wheras dbg.values // would be repeatedly inserted before the first instruction. To replicate diff --git a/llvm/unittests/IR/IRBuilderTest.cpp b/llvm/unittests/IR/IRBuilderTest.cpp index aadae5287c38..3a7ba924792e 100644 --- a/llvm/unittests/IR/IRBuilderTest.cpp +++ b/llvm/unittests/IR/IRBuilderTest.cpp @@ -1003,8 +1003,18 @@ TEST_F(IRBuilderTest, DIBuilder) { EXPECT_TRUE(verifyModule(*M)); }; + // Test in new-debug mode. + EXPECT_TRUE(M->IsNewDbgInfoFormat); RunTest(); + + // Test in old-debug mode. + // Reset the test then call convertFromNewDbgValues to flip the flag + // on the test's Module, Function and BasicBlock. TearDown(); + SetUp(); + M->convertFromNewDbgValues(); + EXPECT_FALSE(M->IsNewDbgInfoFormat); + RunTest(); } TEST_F(IRBuilderTest, createArtificialSubprogram) {