From: Sanjoy Das Date: Thu, 24 Mar 2016 18:57:39 +0000 (+0000) Subject: [Statepoints] Fix yet another issue around gc pointer uniqueing X-Git-Tag: llvmorg-3.9.0-rc1~11005 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=c0c59fe14ea324d69a09d12bf2de2f543b990ac2;p=platform%2Fupstream%2Fllvm.git [Statepoints] Fix yet another issue around gc pointer uniqueing Given that StatepointLowering now uniques derived pointers before putting them in the per-statepoint spill map, we may end up with missing entries for derived pointers when we visit a gc.relocate on a pointer that was de-duplicated away. Fix this by keeping two maps, one mapping gc pointers to their de-duplicated values, and one mapping a de-duplicated value to the slot it is spilled in. llvm-svn: 264320 --- diff --git a/llvm/include/llvm/CodeGen/FunctionLoweringInfo.h b/llvm/include/llvm/CodeGen/FunctionLoweringInfo.h index e457080..b916c30 100644 --- a/llvm/include/llvm/CodeGen/FunctionLoweringInfo.h +++ b/llvm/include/llvm/CodeGen/FunctionLoweringInfo.h @@ -81,13 +81,35 @@ public: DenseMap CatchPadExceptionPointers; /// Keep track of frame indices allocated for statepoints as they could be - /// used across basic block boundaries. Key of the map is statepoint - /// instruction, value is a map from spilled llvm Value to the optional stack - /// stack slot index. If optional is unspecified it means that we have - /// visited this value but didn't spill it. - typedef DenseMap> StatepointSpilledValueMapTy; - DenseMap - StatepointRelocatedValues; + /// used across basic block boundaries. This struct is more complex than a + /// simple map because the stateopint lowering code de-duplicates gc pointers + /// based on their SDValue (so %p and (bitcast %p to T) will get the same + /// slot), and we track that here. + + struct StatepointSpillMap { + typedef DenseMap> SlotMapTy; + + /// Maps uniqued llvm IR values to the slots they were spilled in. If a + /// value is mapped to None it means we visited the value but didn't spill + /// it (because it was a constant, for instance). + SlotMapTy SlotMap; + + /// Maps llvm IR values to the values they were de-duplicated to. + DenseMap DuplicateMap; + + SlotMapTy::const_iterator find(const Value *V) const { + auto DuplIt = DuplicateMap.find(V); + if (DuplIt != DuplicateMap.end()) + V = DuplIt->second; + return SlotMap.find(V); + } + + SlotMapTy::const_iterator end() const { return SlotMap.end(); } + }; + + /// Maps gc.statepoint instructions to their corresponding StatepointSpillMap + /// instances. + DenseMap StatepointSpillMaps; /// StaticAllocaMap - Keep track of frame indices for fixed sized allocas in /// the entry block. This allows the allocas to be efficiently referenced diff --git a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp index e5c7ade..9a99968 100644 --- a/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/FunctionLoweringInfo.cpp @@ -356,7 +356,7 @@ void FunctionLoweringInfo::clear() { ByValArgFrameIndexMap.clear(); RegFixups.clear(); StatepointStackSlots.clear(); - StatepointRelocatedValues.clear(); + StatepointSpillMaps.clear(); PreferredExtendType.clear(); } diff --git a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp index 56035d4..f50d7d0 100644 --- a/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp +++ b/llvm/lib/CodeGen/SelectionDAG/StatepointLowering.cpp @@ -124,7 +124,7 @@ static Optional findPreviousSpillSlot(const Value *Val, // Spill location is known for gc relocates if (const auto *Relocate = dyn_cast(Val)) { const auto &SpillMap = - Builder.FuncInfo.StatepointRelocatedValues[Relocate->getStatepoint()]; + Builder.FuncInfo.StatepointSpillMaps[Relocate->getStatepoint()]; auto It = SpillMap.find(Relocate->getDerivedPtr()); if (It == SpillMap.end()) @@ -249,22 +249,26 @@ static void removeDuplicateGCPtrs(SmallVectorImpl &Bases, SmallVectorImpl &Ptrs, SmallVectorImpl &Relocs, - SelectionDAGBuilder &Builder) { - - // This is horribly inefficient, but I don't care right now - SmallSet Seen; + SelectionDAGBuilder &Builder, + FunctionLoweringInfo::StatepointSpillMap &SSM) { + DenseMap Seen; SmallVector NewBases, NewPtrs; SmallVector NewRelocs; for (size_t i = 0, e = Ptrs.size(); i < e; i++) { SDValue SD = Builder.getValue(Ptrs[i]); - // Only add non-duplicates - if (Seen.count(SD) == 0) { + auto SeenIt = Seen.find(SD); + + if (SeenIt == Seen.end()) { + // Only add non-duplicates NewBases.push_back(Bases[i]); NewPtrs.push_back(Ptrs[i]); NewRelocs.push_back(Relocs[i]); + Seen[SD] = Ptrs[i]; + } else { + // Duplicate pointer found, note in SSM and move on: + SSM.DuplicateMap[Ptrs[i]] = SeenIt->second; } - Seen.insert(SD); } assert(Bases.size() >= NewBases.size()); assert(Ptrs.size() >= NewPtrs.size()); @@ -499,7 +503,7 @@ lowerStatepointMetaArgs(SmallVectorImpl &Ops, // This can not be embedded in lowering loops as we need to record *all* // values, while previous loops account only values with unique SDValues. const Instruction *StatepointInstr = SI.StatepointInstr; - auto &SpillMap = Builder.FuncInfo.StatepointRelocatedValues[StatepointInstr]; + auto &SpillMap = Builder.FuncInfo.StatepointSpillMaps[StatepointInstr]; for (const GCRelocateInst *Relocate : SI.GCRelocates) { const Value *V = Relocate->getDerivedPtr(); @@ -507,7 +511,7 @@ lowerStatepointMetaArgs(SmallVectorImpl &Ops, SDValue Loc = Builder.StatepointLowering.getLocation(SDV); if (Loc.getNode()) { - SpillMap[V] = cast(Loc)->getIndex(); + SpillMap.SlotMap[V] = cast(Loc)->getIndex(); } else { // Record value as visited, but not spilled. This is case for allocas // and constants. For this values we can avoid emitting spill load while @@ -515,7 +519,7 @@ lowerStatepointMetaArgs(SmallVectorImpl &Ops, // Actually we do not need to record them in this map at all. // We do this only to check that we are not relocating any unvisited // value. - SpillMap[V] = None; + SpillMap.SlotMap[V] = None; // Default llvm mechanisms for exporting values which are used in // different basic blocks does not work for gc relocates. @@ -551,7 +555,8 @@ SDValue SelectionDAGBuilder::LowerAsSTATEPOINT( // input. Also has the effect of removing duplicates in the original // llvm::Value input list as well. This is a useful optimization for // reducing the size of the StackMap section. It has no other impact. - removeDuplicateGCPtrs(SI.Bases, SI.Ptrs, SI.GCRelocates, *this); + removeDuplicateGCPtrs(SI.Bases, SI.Ptrs, SI.GCRelocates, *this, + FuncInfo.StatepointSpillMaps[SI.StatepointInstr]); assert(SI.Bases.size() == SI.Ptrs.size() && SI.Ptrs.size() == SI.GCRelocates.size()); @@ -886,12 +891,10 @@ void SelectionDAGBuilder::visitGCRelocate(const GCRelocateInst &Relocate) { const Value *DerivedPtr = Relocate.getDerivedPtr(); SDValue SD = getValue(DerivedPtr); - FunctionLoweringInfo::StatepointSpilledValueMapTy &SpillMap = - FuncInfo.StatepointRelocatedValues[Relocate.getStatepoint()]; - - // We should have recorded location for this pointer - assert(SpillMap.count(DerivedPtr) && "Relocating not lowered gc value"); - Optional DerivedPtrLocation = SpillMap[DerivedPtr]; + auto &SpillMap = FuncInfo.StatepointSpillMaps[Relocate.getStatepoint()]; + auto SlotIt = SpillMap.find(DerivedPtr); + assert(SlotIt != SpillMap.end() && "Relocating not lowered gc value"); + Optional DerivedPtrLocation = SlotIt->second; // We didn't need to spill these special cases (constants and allocas). // See the handling in spillIncomingValueForStatepoint for detail. diff --git a/llvm/test/CodeGen/X86/statepoint-duplicate-gcrelocate.ll b/llvm/test/CodeGen/X86/statepoint-uniqueing.ll similarity index 50% rename from llvm/test/CodeGen/X86/statepoint-duplicate-gcrelocate.ll rename to llvm/test/CodeGen/X86/statepoint-uniqueing.ll index 47847aa..e791bc6 100644 --- a/llvm/test/CodeGen/X86/statepoint-duplicate-gcrelocate.ll +++ b/llvm/test/CodeGen/X86/statepoint-uniqueing.ll @@ -9,12 +9,23 @@ target triple = "x86_64-pc-linux-gnu" declare void @f() declare token @llvm.experimental.gc.statepoint.p0f_isVoidf(i64, i32, void ()*, i32, i32, ...) declare i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token, i32, i32) #3 +declare i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token, i32, i32) #3 -define void @test(i32 addrspace(1)* %ptr) gc "statepoint-example" { -; CHECK-LABEL: test +define void @test_gcrelocate_uniqueing(i32 addrspace(1)* %ptr) gc "statepoint-example" { +; CHECK-LABEL: test_gcrelocate_uniqueing %tok = tail call token (i64, i32, void ()*, i32, i32, ...) @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @f, i32 0, i32 0, i32 0, i32 2, i32 addrspace(1)* %ptr, i32 undef, i32 addrspace(1)* %ptr, i32 addrspace(1)* %ptr) %a = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 9, i32 9) %b = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 10, i32 10) ret void } + +define void @test_gcptr_uniqueing(i32 addrspace(1)* %ptr) gc "statepoint-example" { +; CHECK-LABEL: test_gcptr_uniqueing + %ptr2 = bitcast i32 addrspace(1)* %ptr to i8 addrspace(1)* + %tok = tail call token (i64, i32, void ()*, i32, i32, ...) + @llvm.experimental.gc.statepoint.p0f_isVoidf(i64 0, i32 0, void ()* @f, i32 0, i32 0, i32 0, i32 2, i32 addrspace(1)* %ptr, i32 undef, i32 addrspace(1)* %ptr, i8 addrspace(1)* %ptr2) + %a = call i32 addrspace(1)* @llvm.experimental.gc.relocate.p1i32(token %tok, i32 9, i32 9) + %b = call i8 addrspace(1)* @llvm.experimental.gc.relocate.p1i8(token %tok, i32 10, i32 10) + ret void +}