From f464627f2828b3496456a41b5697f31c6756431f Mon Sep 17 00:00:00 2001 From: Dehao Chen Date: Mon, 2 Oct 2017 18:13:14 +0000 Subject: [PATCH] Update getMergedLocation to check the instruction type and merge properly. Summary: If the merged instruction is call instruction, we need to set the scope to the closes common scope between 2 locations, otherwise it will cause trouble when the call is getting inlined. Reviewers: dblaikie, aprantl Reviewed By: dblaikie, aprantl Subscribers: llvm-commits, sanjoy Differential Revision: https://reviews.llvm.org/D37877 llvm-svn: 314694 --- llvm/include/llvm/IR/DebugInfoMetadata.h | 12 ++++++--- llvm/include/llvm/IR/Instruction.h | 15 +++++++++++ llvm/lib/IR/DebugInfo.cpp | 23 +++++++++++++++++ llvm/lib/Transforms/IPO/FunctionImport.cpp | 4 +++ .../InstCombine/InstCombineInternal.h | 4 +-- .../InstCombineLoadStoreAlloca.cpp | 3 +-- .../Transforms/InstCombine/InstCombinePHI.cpp | 25 ++++++++++--------- llvm/lib/Transforms/Utils/SimplifyCFG.cpp | 25 ++++++++----------- .../Transforms/SimplifyCFG/remove-debug.ll | 22 +++++++++------- 9 files changed, 90 insertions(+), 43 deletions(-) diff --git a/llvm/include/llvm/IR/DebugInfoMetadata.h b/llvm/include/llvm/IR/DebugInfoMetadata.h index e18395781e98..bee8cf8a39d9 100644 --- a/llvm/include/llvm/IR/DebugInfoMetadata.h +++ b/llvm/include/llvm/IR/DebugInfoMetadata.h @@ -1417,11 +1417,15 @@ public: /// could create a location with a new discriminator. If they are from /// different files/lines the location is ambiguous and can't be /// represented in a single line entry. In this case, no location - /// should be set. + /// should be set, unless the merged instruction is a call, which we will + /// set the merged debug location as line 0 of the nearest common scope + /// where 2 locations are inlined from. This only applies to Instruction, + /// For MachineInstruction, as it is post-inline, we will treat the call + /// instruction the same way as other instructions. /// - /// Currently the function does not create a new location. If the locations - /// are the same, or cannot be discriminated, the first location is returned. - /// Otherwise an empty location will be used. + /// This should only be used by MachineInstruction because call can be + /// treated the same as other instructions. Otherwise, use + /// \p applyMergedLocation instead. static const DILocation *getMergedLocation(const DILocation *LocA, const DILocation *LocB) { if (LocA && LocB && (LocA == LocB || !LocA->canDiscriminate(*LocB))) diff --git a/llvm/include/llvm/IR/Instruction.h b/llvm/include/llvm/IR/Instruction.h index 0cf8003423f9..66b1e7e01fe4 100644 --- a/llvm/include/llvm/IR/Instruction.h +++ b/llvm/include/llvm/IR/Instruction.h @@ -377,6 +377,21 @@ public: /// V and this instruction. void andIRFlags(const Value *V); + /// Merge 2 debug locations and apply it to the Instruction. If the + /// instruction is a CallIns, we need to traverse the inline chain to find + /// the common scope. This is not efficient for N-way merging as each time + /// you merge 2 iterations, you need to rebuild the hashmap to find the + /// common scope. However, we still choose this API because: + /// 1) Simplicity: it takes 2 locations instead of a list of locations. + /// 2) In worst case, it increases the complexity from O(N*I) to + /// O(2*N*I), where N is # of Instructions to merge, and I is the + /// maximum level of inline stack. So it is still linear. + /// 3) Merging of call instructions should be extremely rare in real + /// applications, thus the N-way merging should be in code path. + /// The DebugLoc attached to this instruction will be overwritten by the + /// merged DebugLoc. + void applyMergedLocation(const DILocation *LocA, const DILocation *LocB); + private: /// Return true if we have an entry in the on-the-side metadata hash. bool hasMetadataHashEntry() const { diff --git a/llvm/lib/IR/DebugInfo.cpp b/llvm/lib/IR/DebugInfo.cpp index 1dc6c5bdd51f..289798648b52 100644 --- a/llvm/lib/IR/DebugInfo.cpp +++ b/llvm/lib/IR/DebugInfo.cpp @@ -669,3 +669,26 @@ unsigned llvm::getDebugMetadataVersionFromModule(const Module &M) { return Val->getZExtValue(); return 0; } + +void Instruction::applyMergedLocation(const DILocation *LocA, + const DILocation *LocB) { + if (LocA && LocB && (LocA == LocB || !LocA->canDiscriminate(*LocB))) { + setDebugLoc(LocA); + return; + } + if (!LocA || !LocB || !isa(this)) { + setDebugLoc(nullptr); + return; + } + SmallPtrSet InlinedLocationsA; + for (DILocation *L = LocA->getInlinedAt(); L; L = L->getInlinedAt()) + InlinedLocationsA.insert(L); + const DILocation *Result = LocB; + for (DILocation *L = LocB->getInlinedAt(); L; L = L->getInlinedAt()) { + Result = L; + if (InlinedLocationsA.count(L)) + break; + } + setDebugLoc(DILocation::get( + Result->getContext(), 0, 0, Result->getScope(), Result->getInlinedAt())); +} diff --git a/llvm/lib/Transforms/IPO/FunctionImport.cpp b/llvm/lib/Transforms/IPO/FunctionImport.cpp index 670a84862e0a..55599c9149d5 100644 --- a/llvm/lib/Transforms/IPO/FunctionImport.cpp +++ b/llvm/lib/Transforms/IPO/FunctionImport.cpp @@ -463,6 +463,10 @@ void llvm::computeDeadSymbols( // Make value live and add it to the worklist if it was not live before. // FIXME: we should only make the prevailing copy live here auto visit = [&](ValueInfo VI) { + for (auto &S : VI.getSummaryList()) + S->setLive(true); + ++LiveSymbols; + Worklist.push_back(VI); // FIXME: If we knew which edges were created for indirect call profiles, // we could skip them here. Any that are live should be reached via // other edges, e.g. reference edges. Otherwise, using a profile collected diff --git a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h index 22edcfa04441..16caef8fe4bd 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineInternal.h +++ b/llvm/lib/Transforms/InstCombine/InstCombineInternal.h @@ -671,9 +671,9 @@ private: Instruction *FoldPHIArgLoadIntoPHI(PHINode &PN); Instruction *FoldPHIArgZextsIntoPHI(PHINode &PN); - /// Helper function for FoldPHIArgXIntoPHI() to get debug location for the + /// Helper function for FoldPHIArgXIntoPHI() to set debug location for the /// folded operation. - DebugLoc PHIArgMergedDebugLoc(PHINode &PN); + void PHIArgMergedDebugLoc(Instruction *Inst, PHINode &PN); Instruction *foldGEPICmp(GEPOperator *GEPLHS, Value *RHS, ICmpInst::Predicate Cond, Instruction &I); diff --git a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp index 451036545741..5d2402361ad3 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp @@ -1544,8 +1544,7 @@ bool InstCombiner::SimplifyStoreAtEndOfBlock(StoreInst &SI) { SI.getSyncScopeID()); InsertNewInstBefore(NewSI, *BBI); // The debug locations of the original instructions might differ; merge them. - NewSI->setDebugLoc(DILocation::getMergedLocation(SI.getDebugLoc(), - OtherStore->getDebugLoc())); + NewSI->applyMergedLocation(SI.getDebugLoc(), OtherStore->getDebugLoc()); // If the two stores had AA tags, merge them. AAMDNodes AATags; diff --git a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp index 0011412c2bf4..2f31ede32acd 100644 --- a/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp +++ b/llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp @@ -27,16 +27,17 @@ using namespace llvm::PatternMatch; /// The PHI arguments will be folded into a single operation with a PHI node /// as input. The debug location of the single operation will be the merged /// locations of the original PHI node arguments. -DebugLoc InstCombiner::PHIArgMergedDebugLoc(PHINode &PN) { +void InstCombiner::PHIArgMergedDebugLoc(Instruction *Inst, PHINode &PN) { auto *FirstInst = cast(PN.getIncomingValue(0)); - const DILocation *Loc = FirstInst->getDebugLoc(); + Inst->setDebugLoc(FirstInst->getDebugLoc()); + // We do not expect a CallInst here, otherwise, N-way merging of DebugLoc + // will be inefficient. + assert(!isa(Inst)); for (unsigned i = 1; i != PN.getNumIncomingValues(); ++i) { auto *I = cast(PN.getIncomingValue(i)); - Loc = DILocation::getMergedLocation(Loc, I->getDebugLoc()); + Inst->applyMergedLocation(Inst->getDebugLoc(), I->getDebugLoc()); } - - return Loc; } /// If we have something like phi [add (a,b), add(a,c)] and if a/b/c and the @@ -117,7 +118,7 @@ Instruction *InstCombiner::FoldPHIArgBinOpIntoPHI(PHINode &PN) { if (CmpInst *CIOp = dyn_cast(FirstInst)) { CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(), CIOp->getPredicate(), LHSVal, RHSVal); - NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(NewCI, PN); return NewCI; } @@ -130,7 +131,7 @@ Instruction *InstCombiner::FoldPHIArgBinOpIntoPHI(PHINode &PN) { for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) NewBinOp->andIRFlags(PN.getIncomingValue(i)); - NewBinOp->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(NewBinOp, PN); return NewBinOp; } @@ -239,7 +240,7 @@ Instruction *InstCombiner::FoldPHIArgGEPIntoPHI(PHINode &PN) { GetElementPtrInst::Create(FirstInst->getSourceElementType(), Base, makeArrayRef(FixedOperands).slice(1)); if (AllInBounds) NewGEP->setIsInBounds(); - NewGEP->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(NewGEP, PN); return NewGEP; } @@ -399,7 +400,7 @@ Instruction *InstCombiner::FoldPHIArgLoadIntoPHI(PHINode &PN) { for (Value *IncValue : PN.incoming_values()) cast(IncValue)->setVolatile(false); - NewLI->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(NewLI, PN); return NewLI; } @@ -565,7 +566,7 @@ Instruction *InstCombiner::FoldPHIArgOpIntoPHI(PHINode &PN) { if (CastInst *FirstCI = dyn_cast(FirstInst)) { CastInst *NewCI = CastInst::Create(FirstCI->getOpcode(), PhiVal, PN.getType()); - NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(NewCI, PN); return NewCI; } @@ -576,14 +577,14 @@ Instruction *InstCombiner::FoldPHIArgOpIntoPHI(PHINode &PN) { for (unsigned i = 1, e = PN.getNumIncomingValues(); i != e; ++i) BinOp->andIRFlags(PN.getIncomingValue(i)); - BinOp->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(BinOp, PN); return BinOp; } CmpInst *CIOp = cast(FirstInst); CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(), CIOp->getPredicate(), PhiVal, ConstantOp); - NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN)); + PHIArgMergedDebugLoc(NewCI, PN); return NewCI; } diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp index d3e7d70b1a9f..cb24bb311372 100644 --- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp +++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp @@ -1277,9 +1277,7 @@ static bool HoistThenElseCodeToIf(BranchInst *BI, // I1 and I2 are being combined into a single instruction. Its debug // location is the merged locations of the original instructions. - if (!isa(I1)) - I1->setDebugLoc( - DILocation::getMergedLocation(I1->getDebugLoc(), I2->getDebugLoc())); + I1->applyMergedLocation(I1->getDebugLoc(), I2->getDebugLoc()); I2->eraseFromParent(); Changed = true; @@ -1533,20 +1531,20 @@ static bool sinkLastInstruction(ArrayRef Blocks) { I0->getOperandUse(O).set(NewOperands[O]); I0->moveBefore(&*BBEnd->getFirstInsertionPt()); - // The debug location for the "common" instruction is the merged locations of - // all the commoned instructions. We start with the original location of the - // "common" instruction and iteratively merge each location in the loop below. - const DILocation *Loc = I0->getDebugLoc(); - // Update metadata and IR flags, and merge debug locations. for (auto *I : Insts) if (I != I0) { - Loc = DILocation::getMergedLocation(Loc, I->getDebugLoc()); + // The debug location for the "common" instruction is the merged locations + // of all the commoned instructions. We start with the original location + // of the "common" instruction and iteratively merge each location in the + // loop below. + // This is an N-way merge, which will be inefficient if I0 is a CallInst. + // However, as N-way merge for CallInst is rare, so we use simplified API + // instead of using complex API for N-way merge. + I0->applyMergedLocation(I0->getDebugLoc(), I->getDebugLoc()); combineMetadataForCSE(I0, I); I0->andIRFlags(I); } - if (!isa(I0)) - I0->setDebugLoc(Loc); if (!isa(I0)) { // canSinkLastInstruction checked that all instructions were used by @@ -2030,9 +2028,8 @@ static bool SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB, Value *S = Builder.CreateSelect( BrCond, TrueV, FalseV, TrueV->getName() + "." + FalseV->getName(), BI); SpeculatedStore->setOperand(0, S); - SpeculatedStore->setDebugLoc( - DILocation::getMergedLocation( - BI->getDebugLoc(), SpeculatedStore->getDebugLoc())); + SpeculatedStore->applyMergedLocation(BI->getDebugLoc(), + SpeculatedStore->getDebugLoc()); } // Metadata can be dependent on the condition we are hoisting above. diff --git a/llvm/test/Transforms/SimplifyCFG/remove-debug.ll b/llvm/test/Transforms/SimplifyCFG/remove-debug.ll index d4b2373ebf82..dcc03bdf15f1 100644 --- a/llvm/test/Transforms/SimplifyCFG/remove-debug.ll +++ b/llvm/test/Transforms/SimplifyCFG/remove-debug.ll @@ -2,14 +2,9 @@ ; TODO: Track the acutal DebugLoc of the hoisted instruction when no-line ; DebugLoc is supported (https://reviews.llvm.org/D24180) -; CHECK: line: 6 -; CHECK-NOT: line: 7 -; CHECK: line: 8 -; CHECK: line: 9 -; CHECK-NOT: line: 10 -; CHECK: line: 11 -; Checks if the debug info for hoisted "x = i" is removed +; Checks if the debug info for hoisted "x = i" is removed and +; the debug info for hoisted "bar()" is set as line 0 ; int x; ; void bar(); ; void baz(); @@ -20,6 +15,7 @@ ; bar(); ; } else { ; x = i; +; bar(); ; baz(); ; } ; } @@ -30,6 +26,10 @@ target triple = "x86_64-unknown-linux-gnu" ; Function Attrs: uwtable define void @_Z3fooi(i32) #0 !dbg !6 { +; CHECK: load i32, i32* %2, align 4, !tbaa +; CHECK: store i32 %5, i32* @x, align 4, !tbaa +; CHECK: call void @_Z3barv(), !dbg ![[BAR:[0-9]+]] +; CHECK: call void @_Z3bazv(), !dbg ![[BAZ:[0-9]+]] %2 = alloca i32, align 4 store i32 %0, i32* %2, align 4, !tbaa !8 %3 = load i32, i32* %2, align 4, !dbg !12, !tbaa !8 @@ -45,7 +45,8 @@ define void @_Z3fooi(i32) #0 !dbg !6 { ;