[llvm] r314694 - Update getMergedLocation to check the instruction type and merge properly.
Dehao Chen via llvm-commits
llvm-commits at lists.llvm.org
Tue Oct 3 08:48:44 PDT 2017
Oops, sorry, this change was not intended. I'll revert that part now.
Thanks,
Dehao
On Mon, Oct 2, 2017 at 6:06 PM, Peter Collingbourne <peter at pcc.me.uk> wrote:
> Hi Dehao,
>
> On Mon, Oct 2, 2017 at 11:13 AM, Dehao Chen via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> Author: dehao
>> Date: Mon Oct 2 11:13:14 2017
>> New Revision: 314694
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=314694&view=rev
>> Log:
>> 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
>>
>> Modified:
>> llvm/trunk/include/llvm/IR/DebugInfoMetadata.h
>> llvm/trunk/include/llvm/IR/Instruction.h
>> llvm/trunk/lib/IR/DebugInfo.cpp
>> llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>> llvm/trunk/lib/Transforms/InstCombine/InstCombineInternal.h
>> llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> llvm/trunk/lib/Transforms/InstCombine/InstCombinePHI.cpp
>> llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>> llvm/trunk/test/Transforms/SimplifyCFG/remove-debug.ll
>>
>> Modified: llvm/trunk/include/llvm/IR/DebugInfoMetadata.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>> IR/DebugInfoMetadata.h?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/include/llvm/IR/DebugInfoMetadata.h (original)
>> +++ llvm/trunk/include/llvm/IR/DebugInfoMetadata.h Mon Oct 2 11:13:14
>> 2017
>> @@ -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)))
>>
>> Modified: llvm/trunk/include/llvm/IR/Instruction.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/
>> IR/Instruction.h?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/include/llvm/IR/Instruction.h (original)
>> +++ llvm/trunk/include/llvm/IR/Instruction.h Mon Oct 2 11:13:14 2017
>> @@ -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 {
>>
>> Modified: llvm/trunk/lib/IR/DebugInfo.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DebugI
>> nfo.cpp?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/IR/DebugInfo.cpp (original)
>> +++ llvm/trunk/lib/IR/DebugInfo.cpp Mon Oct 2 11:13:14 2017
>> @@ -669,3 +669,26 @@ unsigned llvm::getDebugMetadataVersionFr
>> 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<CallInst>(this)) {
>> + setDebugLoc(nullptr);
>> + return;
>> + }
>> + SmallPtrSet<DILocation *, 5> 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()));
>> +}
>>
>> Modified: llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>> s/IPO/FunctionImport.cpp?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp (original)
>> +++ llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp Mon Oct 2 11:13:14
>> 2017
>> @@ -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);
>>
>
> This part of your change doesn't seem right, and I strongly suspect that
> it is causing OOM errors on the sanitizer-windows bot.
> http://lab.llvm.org:8011/builders/sanitizer-windows/builds/17422
> Can you please take a look?
>
> Peter
>
>
>> // 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
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineInternal.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>> s/InstCombine/InstCombineInternal.h?rev=314694&r1=314693&r2=
>> 314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineInternal.h
>> (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineInternal.h Mon Oct
>> 2 11:13:14 2017
>> @@ -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);
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAl
>> loca.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>> s/InstCombine/InstCombineLoadStoreAlloca.cpp?rev=314694&r1=
>> 314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
>> Mon Oct 2 11:13:14 2017
>> @@ -1544,8 +1544,7 @@ bool InstCombiner::SimplifyStoreAtEndOfB
>> 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;
>>
>> Modified: llvm/trunk/lib/Transforms/InstCombine/InstCombinePHI.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>> s/InstCombine/InstCombinePHI.cpp?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/InstCombine/InstCombinePHI.cpp (original)
>> +++ llvm/trunk/lib/Transforms/InstCombine/InstCombinePHI.cpp Mon Oct 2
>> 11:13:14 2017
>> @@ -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<Instruction>(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<CallInst>(Inst));
>>
>> for (unsigned i = 1; i != PN.getNumIncomingValues(); ++i) {
>> auto *I = cast<Instruction>(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::FoldPHIArgBin
>> if (CmpInst *CIOp = dyn_cast<CmpInst>(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::FoldPHIArgBin
>> 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::FoldPHIArgGEP
>> 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::FoldPHIArgLoa
>> for (Value *IncValue : PN.incoming_values())
>> cast<LoadInst>(IncValue)->setVolatile(false);
>>
>> - NewLI->setDebugLoc(PHIArgMergedDebugLoc(PN));
>> + PHIArgMergedDebugLoc(NewLI, PN);
>> return NewLI;
>> }
>>
>> @@ -565,7 +566,7 @@ Instruction *InstCombiner::FoldPHIArgOpI
>> if (CastInst *FirstCI = dyn_cast<CastInst>(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::FoldPHIArgOpI
>> 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<CmpInst>(FirstInst);
>> CmpInst *NewCI = CmpInst::Create(CIOp->getOpcode(),
>> CIOp->getPredicate(),
>> PhiVal, ConstantOp);
>> - NewCI->setDebugLoc(PHIArgMergedDebugLoc(PN));
>> + PHIArgMergedDebugLoc(NewCI, PN);
>> return NewCI;
>> }
>>
>>
>> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transform
>> s/Utils/SimplifyCFG.cpp?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp (original)
>> +++ llvm/trunk/lib/Transforms/Utils/SimplifyCFG.cpp Mon Oct 2 11:13:14
>> 2017
>> @@ -1277,9 +1277,7 @@ static bool HoistThenElseCodeToIf(Branch
>>
>> // I1 and I2 are being combined into a single instruction. Its debug
>> // location is the merged locations of the original instructions.
>> - if (!isa<CallInst>(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
>> 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<CallInst>(I0))
>> - I0->setDebugLoc(Loc);
>>
>> if (!isa<StoreInst>(I0)) {
>> // canSinkLastInstruction checked that all instructions were used by
>> @@ -2030,9 +2028,8 @@ static bool SpeculativelyExecuteBB(Branc
>> 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.
>>
>> Modified: llvm/trunk/test/Transforms/SimplifyCFG/remove-debug.ll
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transfor
>> ms/SimplifyCFG/remove-debug.ll?rev=314694&r1=314693&r2=314694&view=diff
>> ============================================================
>> ==================
>> --- llvm/trunk/test/Transforms/SimplifyCFG/remove-debug.ll (original)
>> +++ llvm/trunk/test/Transforms/SimplifyCFG/remove-debug.ll Mon Oct 2
>> 11:13:14 2017
>> @@ -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-gn
>>
>> ; 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 {
>> ; <label>:7:
>> %8 = load i32, i32* %2, align 4, !dbg !18, !tbaa !8
>> store i32 %8, i32* @x, align 4, !dbg !19, !tbaa !8
>> - call void @_Z3bazv(), !dbg !20
>> + call void @_Z3barv(), !dbg !20
>> + call void @_Z3bazv(), !dbg !21
>> br label %9
>>
>> ; <label>:9:
>> @@ -59,6 +60,8 @@ declare void @_Z3bazv() #1
>> !llvm.dbg.cu = !{!0}
>> !llvm.module.flags = !{!3, !4}
>>
>> +; CHECK: ![[BAR]] = !DILocation(line: 0
>> +; CHECK: ![[BAZ]] = !DILocation(line: 12, column: 5
>> !0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1)
>> !1 = !DIFile(filename: "a", directory: "b/")
>> !2 = !{}
>> @@ -80,4 +83,5 @@ declare void @_Z3bazv() #1
>> !18 = !DILocation(line: 10, column: 9, scope: !6)
>> !19 = !DILocation(line: 10, column: 7, scope: !6)
>> !20 = !DILocation(line: 11, column: 5, scope: !6)
>> -!21 = !DILocation(line: 13, column: 1, scope: !6)
>> +!21 = !DILocation(line: 12, column: 5, scope: !6)
>> +!22 = !DILocation(line: 14, column: 1, scope: !6)
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
>
>
> --
> --
> Peter
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171003/5f6a445a/attachment.html>
More information about the llvm-commits
mailing list