From 7bdbdc7e64600340f9e67ae3bebfddbb1239335e Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Wed, 4 Sep 2024 16:57:04 +0100 Subject: [PATCH 1/5] [DLCov][NFC] Propagate annotated DebugLocs through transformations In order for DebugLoc coverage testing to work, we have to firstly set annotations for intentionally-empty DebugLocs, and secondly we have to ensure that we do not drop these annotations as we propagate DebugLocs through the compiler. As the annotations exist as part of the DebugLoc class, and not DILocation, they will not survive a DebugLoc->DILocation->DebugLoc roundtrip. Therefore this patch modifies a number of places in the compiler to propagate DebugLocs directly rather than via the underlying DILocation. This has no effect on normal builds; it only ensures that during coverage builds, we do not drop annotations and therefore create false positives. The bulk of these changes are in replacing DILocation::getMergedLocation(s) with a DebugLoc equivalent, and in changing the IRBuilder to store a DebugLoc directly rather than storing DILocations in its general Metadata array. --- .../GlobalISel/LegalizationArtifactCombiner.h | 4 +-- llvm/include/llvm/IR/DebugLoc.h | 25 +++++++++++++++++++ llvm/include/llvm/IR/IRBuilder.h | 21 +++++++++++++--- llvm/include/llvm/IR/Instruction.h | 2 +- llvm/lib/CodeGen/BranchFolding.cpp | 2 +- llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp | 5 ++-- llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp | 2 +- llvm/lib/CodeGen/MachineBasicBlock.cpp | 2 +- llvm/lib/CodeGen/MachineSink.cpp | 4 +-- llvm/lib/IR/DebugInfo.cpp | 4 +-- llvm/lib/IR/DebugLoc.cpp | 21 ++++++++++++++++ llvm/lib/IR/IRBuilder.cpp | 17 ++++--------- llvm/lib/IR/Instruction.cpp | 5 ++-- .../Target/BPF/BPFPreserveStaticOffset.cpp | 11 ++++---- .../InstCombineLoadStoreAlloca.cpp | 4 +-- .../InstCombine/InstructionCombining.cpp | 3 +-- .../Transforms/Scalar/ConstantHoisting.cpp | 2 +- llvm/lib/Transforms/Scalar/LICM.cpp | 8 +++--- .../lib/Transforms/Scalar/SimplifyCFGPass.cpp | 4 +-- llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 6 ++--- 20 files changed, 101 insertions(+), 51 deletions(-) diff --git a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h index 3712a7fa06d9a..22f6a5fde546a 100644 --- a/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h +++ b/llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h @@ -100,8 +100,8 @@ class LegalizationArtifactCombiner { const LLT DstTy = MRI.getType(DstReg); if (isInstLegal({TargetOpcode::G_CONSTANT, {DstTy}})) { auto &CstVal = SrcMI->getOperand(1); - auto *MergedLocation = DILocation::getMergedLocation( - MI.getDebugLoc().get(), SrcMI->getDebugLoc().get()); + auto MergedLocation = + DebugLoc::getMergedLocation(MI.getDebugLoc(), SrcMI->getDebugLoc()); // Set the debug location to the merged location of the SrcMI and the MI // if the aext fold is successful. Builder.setDebugLoc(MergedLocation); diff --git a/llvm/include/llvm/IR/DebugLoc.h b/llvm/include/llvm/IR/DebugLoc.h index c3d0fb80354a4..f62a0343801aa 100644 --- a/llvm/include/llvm/IR/DebugLoc.h +++ b/llvm/include/llvm/IR/DebugLoc.h @@ -142,6 +142,31 @@ namespace llvm { static inline DebugLoc getDropped() { return DebugLoc(); } #endif // LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING + static DebugLoc getMergedLocations(ArrayRef Locs); + static DebugLoc getMergedLocation(DebugLoc LocA, DebugLoc LocB); + + /// If this DebugLoc is non-empty, returns this DebugLoc; otherwise, selects + /// \p Other. + /// In coverage-tracking builds, this also accounts for whether this or + /// \p Other have an annotative DebugLocKind applied, such that if both are + /// empty but exactly one has an annotation, we prefer that annotated + /// location. + DebugLoc orElse(DebugLoc Other) const { + if (*this) + return *this; +#if LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING + if (Other) + return Other; + if (getKind() != DebugLocKind::Normal) + return *this; + if (Other.getKind() != DebugLocKind::Normal) + return Other; + return *this; +#else + return Other; +#endif // LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING + } + /// Get the underlying \a DILocation. /// /// \pre !*this or \c isa(getAsMDNode()). diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h index 0db5179c7a3e4..9560ccf4d2bcf 100644 --- a/llvm/include/llvm/IR/IRBuilder.h +++ b/llvm/include/llvm/IR/IRBuilder.h @@ -113,12 +113,18 @@ class FMFSource { /// Common base class shared among various IRBuilders. class IRBuilderBase { /// Pairs of (metadata kind, MDNode *) that should be added to all newly - /// created instructions, like !dbg metadata. + /// created instructions, excluding !dbg metadata, which is stored in the + // StoredDL field. SmallVector, 2> MetadataToCopy; + // The DebugLoc that will be applied to instructions inserted by this builder. + DebugLoc StoredDL; /// Add or update the an entry (Kind, MD) to MetadataToCopy, if \p MD is not /// null. If \p MD is null, remove the entry with \p Kind. void AddOrRemoveMetadataToCopy(unsigned Kind, MDNode *MD) { + assert(Kind != LLVMContext::MD_dbg && + "MD_dbg metadata must be stored in StoredDL"); + if (!MD) { erase_if(MetadataToCopy, [Kind](const std::pair &KV) { return KV.first == Kind; @@ -238,7 +244,9 @@ class IRBuilderBase { /// Set location information used by debugging information. void SetCurrentDebugLocation(DebugLoc L) { - AddOrRemoveMetadataToCopy(LLVMContext::MD_dbg, L.getAsMDNode()); + // For !dbg metadata attachments, we use DebugLoc instead of the raw MDNode + // to include optional introspection data for use in Debugify. + StoredDL = std::move(L); } /// Set nosanitize metadata. @@ -252,8 +260,12 @@ class IRBuilderBase { /// not on \p Src will be dropped from MetadataToCopy. void CollectMetadataToCopy(Instruction *Src, ArrayRef MetadataKinds) { - for (unsigned K : MetadataKinds) - AddOrRemoveMetadataToCopy(K, Src->getMetadata(K)); + for (unsigned K : MetadataKinds) { + if (K == LLVMContext::MD_dbg) + SetCurrentDebugLocation(Src->getDebugLoc()); + else + AddOrRemoveMetadataToCopy(K, Src->getMetadata(K)); + } } /// Get location information used by debugging information. @@ -267,6 +279,7 @@ class IRBuilderBase { void AddMetadataToInst(Instruction *I) const { for (const auto &KV : MetadataToCopy) I->setMetadata(KV.first, KV.second); + SetInstDebugLocation(I); } /// Get the return type of the current function that we're emitting diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h index 10fc9c1298607..8e1ef24226789 100644 --- a/llvm/include/llvm/IR/Instruction.h +++ b/llvm/include/llvm/IR/Instruction.h @@ -698,7 +698,7 @@ class Instruction : public User, /// applications, thus the N-way merging should be in code path. /// The DebugLoc attached to this instruction will be overwritten by the /// merged DebugLoc. - LLVM_ABI void applyMergedLocation(DILocation *LocA, DILocation *LocB); + LLVM_ABI void applyMergedLocation(DebugLoc LocA, DebugLoc LocB); /// Updates the debug location given that the instruction has been hoisted /// from a block to a predecessor of that block. diff --git a/llvm/lib/CodeGen/BranchFolding.cpp b/llvm/lib/CodeGen/BranchFolding.cpp index e0f7466ceacff..ff9f0ff5d5bc3 100644 --- a/llvm/lib/CodeGen/BranchFolding.cpp +++ b/llvm/lib/CodeGen/BranchFolding.cpp @@ -862,7 +862,7 @@ void BranchFolder::mergeCommonTails(unsigned commonTailIndex) { "Reached BB end within common tail"); } assert(MI.isIdenticalTo(*Pos) && "Expected matching MIIs!"); - DL = DILocation::getMergedLocation(DL, Pos->getDebugLoc()); + DL = DebugLoc::getMergedLocation(DL, Pos->getDebugLoc()); NextCommonInsts[i] = ++Pos; } MI.setDebugLoc(DL); diff --git a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp index 10c72641ce2df..e3e6c72165ebb 100644 --- a/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp +++ b/llvm/lib/CodeGen/GlobalISel/CSEMIRBuilder.cpp @@ -53,8 +53,7 @@ CSEMIRBuilder::getDominatingInstrForID(FoldingSetNodeID &ID, } else if (!dominates(MI, CurrPos)) { // Update the spliced machineinstr's debug location by merging it with the // debug location of the instruction at the insertion point. - auto *Loc = DILocation::getMergedLocation(getDebugLoc().get(), - MI->getDebugLoc().get()); + auto Loc = DebugLoc::getMergedLocation(getDebugLoc(), MI->getDebugLoc()); MI->setDebugLoc(Loc); CurMBB->splice(CurrPos, CurMBB, MI); } @@ -170,7 +169,7 @@ CSEMIRBuilder::generateCopiesIfRequired(ArrayRef DstOps, if (Observer) Observer->changingInstr(*MIB); MIB->setDebugLoc( - DILocation::getMergedLocation(MIB->getDebugLoc(), getDebugLoc())); + DebugLoc::getMergedLocation(MIB->getDebugLoc(), getDebugLoc())); if (Observer) Observer->changedInstr(*MIB); } diff --git a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp index 78cd9bc7891e0..f68420ed66e4b 100644 --- a/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp +++ b/llvm/lib/CodeGen/GlobalISel/LoadStoreOpt.cpp @@ -370,7 +370,7 @@ bool LoadStoreOpt::doSingleStoreMerge(SmallVectorImpl &Stores) { // For each store, compute pairwise merged debug locs. DebugLoc MergedLoc = Stores.front()->getDebugLoc(); for (auto *Store : drop_begin(Stores)) - MergedLoc = DILocation::getMergedLocation(MergedLoc, Store->getDebugLoc()); + MergedLoc = DebugLoc::getMergedLocation(MergedLoc, Store->getDebugLoc()); Builder.setInstr(*Stores.back()); Builder.setDebugLoc(MergedLoc); diff --git a/llvm/lib/CodeGen/MachineBasicBlock.cpp b/llvm/lib/CodeGen/MachineBasicBlock.cpp index ccc164a0881e9..48b406e016c05 100644 --- a/llvm/lib/CodeGen/MachineBasicBlock.cpp +++ b/llvm/lib/CodeGen/MachineBasicBlock.cpp @@ -1574,7 +1574,7 @@ MachineBasicBlock::findBranchDebugLoc() { DL = TI->getDebugLoc(); for (++TI ; TI != end() ; ++TI) if (TI->isBranch()) - DL = DILocation::getMergedLocation(DL, TI->getDebugLoc()); + DL = DebugLoc::getMergedLocation(DL, TI->getDebugLoc()); } return DL; } diff --git a/llvm/lib/CodeGen/MachineSink.cpp b/llvm/lib/CodeGen/MachineSink.cpp index e3f6eda8ff065..8411d5c4b09c8 100644 --- a/llvm/lib/CodeGen/MachineSink.cpp +++ b/llvm/lib/CodeGen/MachineSink.cpp @@ -1611,8 +1611,8 @@ static void performSink(MachineInstr &MI, MachineBasicBlock &SuccToSinkTo, // location to prevent debug-info driven tools from potentially reporting // wrong location information. if (!SuccToSinkTo.empty() && InsertPos != SuccToSinkTo.end()) - MI.setDebugLoc(DILocation::getMergedLocation(MI.getDebugLoc(), - InsertPos->getDebugLoc())); + MI.setDebugLoc(DebugLoc::getMergedLocation(MI.getDebugLoc(), + InsertPos->getDebugLoc())); else MI.setDebugLoc(DebugLoc()); diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp index 7db9891fdbd75..bf54ce08428bb 100644 --- a/llvm/lib/IR/DebugInfo.cpp +++ b/llvm/lib/IR/DebugInfo.cpp @@ -960,8 +960,8 @@ unsigned llvm::getDebugMetadataVersionFromModule(const Module &M) { return 0; } -void Instruction::applyMergedLocation(DILocation *LocA, DILocation *LocB) { - setDebugLoc(DILocation::getMergedLocation(LocA, LocB)); +void Instruction::applyMergedLocation(DebugLoc LocA, DebugLoc LocB) { + setDebugLoc(DebugLoc::getMergedLocation(LocA, LocB)); } void Instruction::mergeDIAssignID( diff --git a/llvm/lib/IR/DebugLoc.cpp b/llvm/lib/IR/DebugLoc.cpp index 0e65ddcec8934..0be6d55d724e0 100644 --- a/llvm/lib/IR/DebugLoc.cpp +++ b/llvm/lib/IR/DebugLoc.cpp @@ -143,6 +143,27 @@ DebugLoc DebugLoc::appendInlinedAt(const DebugLoc &DL, DILocation *InlinedAt, return Last; } +DebugLoc DebugLoc::getMergedLocations(ArrayRef Locs) { + if (Locs.empty()) + return DebugLoc(); + if (Locs.size() == 1) + return Locs[0]; + DebugLoc Merged = Locs[0]; + for (const DebugLoc &DL : llvm::drop_begin(Locs)) { + Merged = getMergedLocation(Merged, DL); + if (!Merged) + break; + } + return Merged; +} +DebugLoc DebugLoc::getMergedLocation(DebugLoc LocA, DebugLoc LocB) { + if (!LocA) + return LocA; + if (!LocB) + return LocB; + return DILocation::getMergedLocation(LocA, LocB); +} + #if !defined(NDEBUG) || defined(LLVM_ENABLE_DUMP) LLVM_DUMP_METHOD void DebugLoc::dump() const { print(dbgs()); } #endif diff --git a/llvm/lib/IR/IRBuilder.cpp b/llvm/lib/IR/IRBuilder.cpp index 580b0af709337..dd9fd6641d121 100644 --- a/llvm/lib/IR/IRBuilder.cpp +++ b/llvm/lib/IR/IRBuilder.cpp @@ -61,19 +61,12 @@ Type *IRBuilderBase::getCurrentFunctionReturnType() const { return BB->getParent()->getReturnType(); } -DebugLoc IRBuilderBase::getCurrentDebugLocation() const { - for (auto &KV : MetadataToCopy) - if (KV.first == LLVMContext::MD_dbg) - return {cast(KV.second)}; - - return {}; -} +DebugLoc IRBuilderBase::getCurrentDebugLocation() const { return StoredDL; } void IRBuilderBase::SetInstDebugLocation(Instruction *I) const { - for (const auto &KV : MetadataToCopy) - if (KV.first == LLVMContext::MD_dbg) { - I->setDebugLoc(DebugLoc(KV.second)); - return; - } + // We prefer to set our current debug location if any has been set, but if + // our debug location is empty and I has a valid location, we shouldn't + // overwrite it. + I->setDebugLoc(StoredDL.orElse(I->getDebugLoc())); } Value *IRBuilderBase::CreateAggregateCast(Value *V, Type *DestTy) { diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp index 18ce16522af90..71944d7f66e6a 100644 --- a/llvm/lib/IR/Instruction.cpp +++ b/llvm/lib/IR/Instruction.cpp @@ -1363,6 +1363,9 @@ void Instruction::swapProfMetadata() { void Instruction::copyMetadata(const Instruction &SrcInst, ArrayRef WL) { + if (WL.empty() || is_contained(WL, LLVMContext::MD_dbg)) + setDebugLoc(getDebugLoc().orElse(SrcInst.getDebugLoc())); + if (!SrcInst.hasMetadata()) return; @@ -1376,8 +1379,6 @@ void Instruction::copyMetadata(const Instruction &SrcInst, if (WL.empty() || WLS.count(MD.first)) setMetadata(MD.first, MD.second); } - if (WL.empty() || WLS.count(LLVMContext::MD_dbg)) - setDebugLoc(SrcInst.getDebugLoc()); } Instruction *Instruction::clone() const { diff --git a/llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp b/llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp index 77bbeab3c2790..222eb19e3eeef 100644 --- a/llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp +++ b/llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp @@ -150,10 +150,10 @@ static CallInst *isGEPAndStore(Value *I) { } template -static DILocation *mergeDILocations(SmallVector &Insns) { - DILocation *Merged = (*Insns.begin())->getDebugLoc(); +static DebugLoc mergeDebugLocs(SmallVector &Insns) { + DebugLoc Merged = (*Insns.begin())->getDebugLoc(); for (T *I : Insns) - Merged = DILocation::getMergedLocation(Merged, I->getDebugLoc()); + Merged = DebugLoc::getMergedLocation(Merged, I->getDebugLoc()); return Merged; } @@ -227,7 +227,7 @@ static Instruction *makeGEPAndLoad(Module *M, GEPChainInfo &GEP, CallInst *Call = makeIntrinsicCall(M, Intrinsic::bpf_getelementptr_and_load, {Load->getType()}, Args); setParamElementType(Call, 0, GEP.SourceElementType); - Call->applyMergedLocation(mergeDILocations(GEP.Members), Load->getDebugLoc()); + Call->applyMergedLocation(mergeDebugLocs(GEP.Members), Load->getDebugLoc()); Call->setName((*GEP.Members.rbegin())->getName()); if (Load->isUnordered()) { Call->setOnlyReadsMemory(); @@ -251,8 +251,7 @@ static Instruction *makeGEPAndStore(Module *M, GEPChainInfo &GEP, setParamElementType(Call, 1, GEP.SourceElementType); if (Store->getValueOperand()->getType()->isPointerTy()) setParamReadNone(Call, 0); - Call->applyMergedLocation(mergeDILocations(GEP.Members), - Store->getDebugLoc()); + Call->applyMergedLocation(mergeDebugLocs(GEP.Members), Store->getDebugLoc()); if (Store->isUnordered()) { Call->setOnlyWritesMemory(); Call->setOnlyAccessesArgMemory(); diff --git a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp index 324e6022f3f05..1d208de75db3b 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp @@ -1581,8 +1581,8 @@ bool InstCombinerImpl::mergeStoreIntoSuccessor(StoreInst &SI) { // Insert a PHI node now if we need it. Value *MergedVal = OtherStore->getValueOperand(); // The debug locations of the original instructions might differ. Merge them. - DebugLoc MergedLoc = DILocation::getMergedLocation(SI.getDebugLoc(), - OtherStore->getDebugLoc()); + DebugLoc MergedLoc = + DebugLoc::getMergedLocation(SI.getDebugLoc(), OtherStore->getDebugLoc()); if (MergedVal != SI.getValueOperand()) { PHINode *PN = PHINode::Create(SI.getValueOperand()->getType(), 2, "storemerge"); diff --git a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp index e261807bbc035..dc2a8cb0115e7 100644 --- a/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp +++ b/llvm/lib/Transforms/InstCombine/InstructionCombining.cpp @@ -5340,8 +5340,7 @@ bool InstCombinerImpl::run() { // We copy the old instruction's DebugLoc to the new instruction, unless // InstCombine already assigned a DebugLoc to it, in which case we // should trust the more specifically selected DebugLoc. - if (!Result->getDebugLoc()) - Result->setDebugLoc(I->getDebugLoc()); + Result->setDebugLoc(Result->getDebugLoc().orElse(I->getDebugLoc())); // We also copy annotation metadata to the new instruction. Result->copyMetadata(*I, LLVMContext::MD_annotation); // Everything uses the new instruction now. diff --git a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp index 07bc623c3dea0..839f5933e09b0 100644 --- a/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp +++ b/llvm/lib/Transforms/Scalar/ConstantHoisting.cpp @@ -883,7 +883,7 @@ bool ConstantHoistingPass::emitBaseConstants(GlobalVariable *BaseGV) { emitBaseConstants(Base, &R); ReBasesNum++; // Use the same debug location as the last user of the constant. - Base->setDebugLoc(DILocation::getMergedLocation( + Base->setDebugLoc(DebugLoc::getMergedLocation( Base->getDebugLoc(), R.User.Inst->getDebugLoc())); } assert(!Base->use_empty() && "The use list is empty!?"); diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp index 9773ef778b690..693d5af40c6b5 100644 --- a/llvm/lib/Transforms/Scalar/LICM.cpp +++ b/llvm/lib/Transforms/Scalar/LICM.cpp @@ -2224,10 +2224,10 @@ bool llvm::promoteLoopAccessesToScalars( }); // Look at all the loop uses, and try to merge their locations. - std::vector LoopUsesLocs; - for (auto *U : LoopUses) - LoopUsesLocs.push_back(U->getDebugLoc().get()); - auto DL = DebugLoc(DILocation::getMergedLocations(LoopUsesLocs)); + std::vector LoopUsesLocs; + for (auto U : LoopUses) + LoopUsesLocs.push_back(U->getDebugLoc()); + auto DL = DebugLoc::getMergedLocations(LoopUsesLocs); // We use the SSAUpdater interface to insert phi nodes as required. SmallVector NewPHIs; diff --git a/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp b/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp index 4e437e9abeb43..d20378ece4eea 100644 --- a/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp +++ b/llvm/lib/Transforms/Scalar/SimplifyCFGPass.cpp @@ -128,7 +128,7 @@ performBlockTailMerging(Function &F, ArrayRef BBs, // Now, go through each block (with the current terminator type) // we've recorded, and rewrite it to branch to the new common block. - DILocation *CommonDebugLoc = nullptr; + DebugLoc CommonDebugLoc; for (BasicBlock *BB : BBs) { auto *Term = BB->getTerminator(); assert(Term->getOpcode() == CanonicalTerm->getOpcode() && @@ -145,7 +145,7 @@ performBlockTailMerging(Function &F, ArrayRef BBs, CommonDebugLoc = Term->getDebugLoc(); else CommonDebugLoc = - DILocation::getMergedLocation(CommonDebugLoc, Term->getDebugLoc()); + DebugLoc::getMergedLocation(CommonDebugLoc, Term->getDebugLoc()); // And turn BB into a block that just unconditionally branches // to the canonical block. diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index e221022bb8361..6a5868c047646 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -2095,11 +2095,11 @@ bool SimplifyCFGOpt::hoistSuccIdenticalTerminatorToSwitchOrIf( // Ensure terminator gets a debug location, even an unknown one, in case // it involves inlinable calls. - SmallVector Locs; + SmallVector Locs; Locs.push_back(I1->getDebugLoc()); for (auto *OtherSuccTI : OtherSuccTIs) Locs.push_back(OtherSuccTI->getDebugLoc()); - NT->setDebugLoc(DILocation::getMergedLocations(Locs)); + NT->setDebugLoc(DebugLoc::getMergedLocations(Locs)); // PHIs created below will adopt NT's merged DebugLoc. IRBuilder Builder(NT); @@ -2895,7 +2895,7 @@ static void mergeCompatibleInvokesImpl(ArrayRef Invokes, MergedDebugLoc = II->getDebugLoc(); else MergedDebugLoc = - DILocation::getMergedLocation(MergedDebugLoc, II->getDebugLoc()); + DebugLoc::getMergedLocation(MergedDebugLoc, II->getDebugLoc()); // And replace the old `invoke` with an unconditionally branch // to the block with the merged `invoke`. From 9210563798a64fb58aaf8a57772f2c8cf8abff2d Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Thu, 1 May 2025 11:19:12 +0100 Subject: [PATCH 2/5] Fix accidentally-inverted 'else' in copyMetadata --- llvm/lib/IR/Instruction.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/llvm/lib/IR/Instruction.cpp b/llvm/lib/IR/Instruction.cpp index 71944d7f66e6a..361ae6d98e9d9 100644 --- a/llvm/lib/IR/Instruction.cpp +++ b/llvm/lib/IR/Instruction.cpp @@ -1364,7 +1364,7 @@ void Instruction::swapProfMetadata() { void Instruction::copyMetadata(const Instruction &SrcInst, ArrayRef WL) { if (WL.empty() || is_contained(WL, LLVMContext::MD_dbg)) - setDebugLoc(getDebugLoc().orElse(SrcInst.getDebugLoc())); + setDebugLoc(SrcInst.getDebugLoc().orElse(getDebugLoc())); if (!SrcInst.hasMetadata()) return; From bd270c72258ff7c0cae56622f71bfaae5c0bd072 Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Tue, 10 Jun 2025 18:04:56 +0100 Subject: [PATCH 3/5] Fix comment, propagate through VPTransformState::get --- llvm/include/llvm/IR/IRBuilder.h | 4 ++-- llvm/lib/Transforms/Vectorize/VPlan.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h index 9560ccf4d2bcf..cfae20ed400be 100644 --- a/llvm/include/llvm/IR/IRBuilder.h +++ b/llvm/include/llvm/IR/IRBuilder.h @@ -114,9 +114,9 @@ class FMFSource { class IRBuilderBase { /// Pairs of (metadata kind, MDNode *) that should be added to all newly /// created instructions, excluding !dbg metadata, which is stored in the - // StoredDL field. + /// StoredDL field. SmallVector, 2> MetadataToCopy; - // The DebugLoc that will be applied to instructions inserted by this builder. + /// The DebugLoc that will be applied to instructions inserted by this builder. DebugLoc StoredDL; /// Add or update the an entry (Kind, MD) to MetadataToCopy, if \p MD is not diff --git a/llvm/lib/Transforms/Vectorize/VPlan.cpp b/llvm/lib/Transforms/Vectorize/VPlan.cpp index 1838562f26b82..b74ef91f26e70 100644 --- a/llvm/lib/Transforms/Vectorize/VPlan.cpp +++ b/llvm/lib/Transforms/Vectorize/VPlan.cpp @@ -395,7 +395,7 @@ void VPTransformState::setDebugLocFrom(DebugLoc DL) { LLVM_DEBUG(dbgs() << "Failed to create new discriminator: " << DIL->getFilename() << " Line: " << DIL->getLine()); } else - Builder.SetCurrentDebugLocation(DIL); + Builder.SetCurrentDebugLocation(DL); } void VPTransformState::packScalarIntoVectorizedValue(const VPValue *Def, From b1b46821498ce688ccaae40692ca664053ce441b Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Tue, 10 Jun 2025 20:22:24 +0100 Subject: [PATCH 4/5] clang-format --- llvm/include/llvm/IR/IRBuilder.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/llvm/include/llvm/IR/IRBuilder.h b/llvm/include/llvm/IR/IRBuilder.h index cfae20ed400be..c86131ef6f3c0 100644 --- a/llvm/include/llvm/IR/IRBuilder.h +++ b/llvm/include/llvm/IR/IRBuilder.h @@ -116,7 +116,8 @@ class IRBuilderBase { /// created instructions, excluding !dbg metadata, which is stored in the /// StoredDL field. SmallVector, 2> MetadataToCopy; - /// The DebugLoc that will be applied to instructions inserted by this builder. + /// The DebugLoc that will be applied to instructions inserted by this + /// builder. DebugLoc StoredDL; /// Add or update the an entry (Kind, MD) to MetadataToCopy, if \p MD is not From 9e66bbe70aab0c8399faa7463f9cb8bc0c28070a Mon Sep 17 00:00:00 2001 From: Stephen Tozer Date: Thu, 12 Jun 2025 10:35:12 +0100 Subject: [PATCH 5/5] Make DebugLoc::getMergedLocation the clear preferred choice --- llvm/include/llvm/IR/DebugInfoMetadata.h | 24 ++++++++++-------------- llvm/include/llvm/IR/DebugLoc.h | 24 ++++++++++++++++++++++-- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h index 02f0a9f677db3..18228b7757897 100644 --- a/llvm/include/llvm/IR/DebugInfoMetadata.h +++ b/llvm/include/llvm/IR/DebugInfoMetadata.h @@ -2437,25 +2437,21 @@ class DILocation : public MDNode { inline std::optional cloneByMultiplyingDuplicationFactor(unsigned DF) const; - /// When two instructions are combined into a single instruction we also - /// need to combine the original locations into a single location. - /// When the locations are the same we can use either location. - /// When they differ, we need a third location which is distinct from either. - /// If they share a common scope, use this scope and compare the line/column - /// pair of the locations with the common scope: - /// * if both match, keep the line and column; - /// * if only the line number matches, keep the line and set the column as 0; - /// * otherwise set line and column as 0. - /// If they do not share a common scope the location is ambiguous and can't be - /// represented in a line entry. In this case, set line and column as 0 and - /// use the scope of any location. - /// - /// \p LocA \p LocB: The locations to be merged. + /// Attempts to merge \p LocA and \p LocB into a single location; see + /// DebugLoc::getMergedLocation for more details. + /// NB: When merging the locations of instructions, prefer to use + /// DebugLoc::getMergedLocation(), as an instruction's DebugLoc may contain + /// additional metadata that will not be preserved when merging the unwrapped + /// DILocations. LLVM_ABI static DILocation *getMergedLocation(DILocation *LocA, DILocation *LocB); /// Try to combine the vector of locations passed as input in a single one. /// This function applies getMergedLocation() repeatedly left-to-right. + /// NB: When merging the locations of instructions, prefer to use + /// DebugLoc::getMergedLocations(), as an instruction's DebugLoc may contain + /// additional metadata that will not be preserved when merging the unwrapped + /// DILocations. /// /// \p Locs: The locations to be merged. LLVM_ABI static DILocation *getMergedLocations(ArrayRef Locs); diff --git a/llvm/include/llvm/IR/DebugLoc.h b/llvm/include/llvm/IR/DebugLoc.h index f62a0343801aa..2fabae9bfc66e 100644 --- a/llvm/include/llvm/IR/DebugLoc.h +++ b/llvm/include/llvm/IR/DebugLoc.h @@ -142,8 +142,28 @@ namespace llvm { static inline DebugLoc getDropped() { return DebugLoc(); } #endif // LLVM_ENABLE_DEBUGLOC_COVERAGE_TRACKING - static DebugLoc getMergedLocations(ArrayRef Locs); - static DebugLoc getMergedLocation(DebugLoc LocA, DebugLoc LocB); + /// When two instructions are combined into a single instruction we also + /// need to combine the original locations into a single location. + /// When the locations are the same we can use either location. + /// When they differ, we need a third location which is distinct from + /// either. If they share a common scope, use this scope and compare the + /// line/column pair of the locations with the common scope: + /// * if both match, keep the line and column; + /// * if only the line number matches, keep the line and set the column as + /// 0; + /// * otherwise set line and column as 0. + /// If they do not share a common scope the location is ambiguous and can't + /// be represented in a line entry. In this case, set line and column as 0 + /// and use the scope of any location. + /// + /// \p LocA \p LocB: The locations to be merged. + LLVM_ABI static DebugLoc getMergedLocation(DebugLoc LocA, DebugLoc LocB); + + /// Try to combine the vector of locations passed as input in a single one. + /// This function applies getMergedLocation() repeatedly left-to-right. + /// + /// \p Locs: The locations to be merged. + LLVM_ABI static DebugLoc getMergedLocations(ArrayRef Locs); /// If this DebugLoc is non-empty, returns this DebugLoc; otherwise, selects /// \p Other.