From 5769cb0ec7f3253e70cc80717dd8de60c6a192c4 Mon Sep 17 00:00:00 2001 From: Yichao Yu Date: Fri, 6 Oct 2017 22:55:11 -0400 Subject: [PATCH] Refine liveness of phi and select If both inputs are rooted then we don't need to root the result. Rename `LoadRefinements` to `Refinements` since it's not load specific anymore. Fix #22421 --- src/llvm-late-gc-lowering.cpp | 77 +++++++++++++++++++++++------------ test/codegen.jl | 32 +++++++++++++++ test/llvmpasses/gcroots.ll | 21 ++++++++++ 3 files changed, 105 insertions(+), 25 deletions(-) diff --git a/src/llvm-late-gc-lowering.cpp b/src/llvm-late-gc-lowering.cpp index db73e73e57df8..b21b4c88c06dc 100644 --- a/src/llvm-late-gc-lowering.cpp +++ b/src/llvm-late-gc-lowering.cpp @@ -255,9 +255,9 @@ struct State { // The result of the local analysis std::map BBStates; - // Load refinement map. All uses of the keys can be combined with uses - // of the value (but not the other way around). - std::map LoadRefinements; + // Refinement map. If all of the values are rooted (-1 means a globally rooted value), + // the key is already rooted (but not the other way around). + std::map> Refinements; // GC preserves map. All safepoints dominated by the map key, but not any // of its uses need to preserve the values listed in the map value. @@ -334,7 +334,7 @@ struct LateLowerGCFrame: public FunctionPass { Function *big_alloc_func; CallInst *ptlsStates; - void MaybeNoteDef(State &S, BBState &BBS, Value *Def, const std::vector &SafepointsSoFar, int RefinedPtr = -2); + void MaybeNoteDef(State &S, BBState &BBS, Value *Def, const std::vector &SafepointsSoFar, SmallVector &&RefinedPtr = SmallVector()); void NoteUse(State &S, BBState &BBS, Value *V, BitVector &Uses); void NoteUse(State &S, BBState &BBS, Value *V) { NoteUse(S, BBS, V, BBS.UpExposedUses); @@ -575,7 +575,7 @@ static void NoteDef(State &S, BBState &BBS, int Num, const std::vector &Saf } } -void LateLowerGCFrame::MaybeNoteDef(State &S, BBState &BBS, Value *Def, const std::vector &SafepointsSoFar, int RefinedPtr) { +void LateLowerGCFrame::MaybeNoteDef(State &S, BBState &BBS, Value *Def, const std::vector &SafepointsSoFar, SmallVector &&RefinedPtr) { int Num = -1; Type *RT = Def->getType(); if (isSpecialPtr(RT)) { @@ -591,8 +591,8 @@ void LateLowerGCFrame::MaybeNoteDef(State &S, BBState &BBS, Value *Def, const st std::vector Nums = NumberVector(S, Def); for (int Num : Nums) { NoteDef(S, BBS, Num, SafepointsSoFar); - if (RefinedPtr != -2) - S.LoadRefinements[Num] = RefinedPtr; + if (!RefinedPtr.empty()) + S.Refinements[Num] = RefinedPtr; } return; } @@ -600,8 +600,8 @@ void LateLowerGCFrame::MaybeNoteDef(State &S, BBState &BBS, Value *Def, const st return; } NoteDef(S, BBS, Num, SafepointsSoFar); - if (RefinedPtr != -2) - S.LoadRefinements[Num] = RefinedPtr; + if (!RefinedPtr.empty()) + S.Refinements[Num] = std::move(RefinedPtr); } static int NoteSafepoint(State &S, BBState &BBS, CallInst *CI) { @@ -814,21 +814,21 @@ State LateLowerGCFrame::LocalScan(Function &F) { // object we're loading from is, so we can refine uses // of this object to uses of the object we're loading // from. - int RefinedPtr = -2; + SmallVector RefinedPtr{}; if (isLoadFromImmut(LI) && isSpecialPtr(LI->getPointerOperand()->getType())) { - RefinedPtr = Number(S, LI->getPointerOperand()); + RefinedPtr.push_back(Number(S, LI->getPointerOperand())); } else if (LI->getType()->isPointerTy() && isSpecialPtr(LI->getType()) && LooksLikeFrameRef(LI->getPointerOperand())) { // Loads from a jlcall argument array - RefinedPtr = -1; + RefinedPtr.push_back(-1); } else if (isLoadFromConstGV(LI)) { // If this is a const load from a global, // we know that the object is a constant as well and doesn't need rooting. - RefinedPtr = -1; + RefinedPtr.push_back(-1); } - MaybeNoteDef(S, BBS, LI, BBS.Safepoints, RefinedPtr); + MaybeNoteDef(S, BBS, LI, BBS.Safepoints, std::move(RefinedPtr)); NoteOperandUses(S, BBS, I, BBS.UpExposedUsesUnrooted); } else if (SelectInst *SI = dyn_cast(&I)) { // We need to insert an extra select for the GC root @@ -837,23 +837,40 @@ State LateLowerGCFrame::LocalScan(Function &F) { if (getValueAddrSpace(SI) != AddressSpace::Tracked) { if (S.AllPtrNumbering.find(SI) != S.AllPtrNumbering.end()) continue; - LiftSelect(S, SI); + auto Num = LiftSelect(S, SI); + if (Num == -1) + continue; + auto SelectBase = cast(S.ReversePtrNumbering[Num]); + SmallVector RefinedPtr{Number(S, SelectBase->getTrueValue()), + Number(S, SelectBase->getFalseValue())}; + S.Refinements[Num] = std::move(RefinedPtr); } else { - MaybeNoteDef(S, BBS, SI, BBS.Safepoints); + SmallVector RefinedPtr{Number(S, SI->getTrueValue()), + Number(S, SI->getFalseValue())}; + MaybeNoteDef(S, BBS, SI, BBS.Safepoints, std::move(RefinedPtr)); NoteOperandUses(S, BBS, I, BBS.UpExposedUsesUnrooted); } } else if (PHINode *Phi = dyn_cast(&I)) { if (!isSpecialPtr(Phi->getType())) { continue; } + auto nIncoming = Phi->getNumIncomingValues(); // We need to insert an extra phi for the GC root if (getValueAddrSpace(Phi) != AddressSpace::Tracked) { if (S.AllPtrNumbering.find(Phi) != S.AllPtrNumbering.end()) continue; - LiftPhi(S, Phi); + auto Num = LiftPhi(S, Phi); + auto lift = cast(S.ReversePtrNumbering[Num]); + SmallVector RefinedPtr(nIncoming); + for (unsigned i = 0; i < nIncoming; ++i) + RefinedPtr[i] = Number(S, lift->getIncomingValue(i)); + S.Refinements[Num] = std::move(RefinedPtr); } else { - MaybeNoteDef(S, BBS, Phi, BBS.Safepoints); - for (unsigned i = 0; i < Phi->getNumIncomingValues(); ++i) { + SmallVector RefinedPtr(nIncoming); + for (unsigned i = 0; i < nIncoming; ++i) + RefinedPtr[i] = Number(S, Phi->getIncomingValue(i)); + MaybeNoteDef(S, BBS, Phi, BBS.Safepoints, std::move(RefinedPtr)); + for (unsigned i = 0; i < nIncoming; ++i) { BBState &IncomingBBS = S.BBStates[Phi->getIncomingBlock(i)]; NoteUse(S, IncomingBBS, Phi->getIncomingValue(i), IncomingBBS.PhiOuts); } @@ -862,14 +879,14 @@ State LateLowerGCFrame::LocalScan(Function &F) { NoteOperandUses(S, BBS, I, BBS.UpExposedUsesUnrooted); } else if (auto *ASCI = dyn_cast(&I)) { if (getValueAddrSpace(ASCI) == AddressSpace::Tracked) { - int RefinedPtr = -2; + SmallVector RefinedPtr{}; auto origin = ASCI->getPointerOperand()->stripPointerCasts(); if (auto LI = dyn_cast(origin)) { if (isLoadFromConstGV(LI)) { - RefinedPtr = -1; + RefinedPtr.push_back(-1); } } - MaybeNoteDef(S, BBS, ASCI, BBS.Safepoints, RefinedPtr); + MaybeNoteDef(S, BBS, ASCI, BBS.Safepoints, std::move(RefinedPtr)); } } else if (auto *AI = dyn_cast(&I)) { if (isSpecialPtr(AI->getAllocatedType()) && !AI->isArrayAllocation() && @@ -982,11 +999,21 @@ void LateLowerGCFrame::ComputeLiveSets(Function &F, State &S) { } // Apply refinements for (int Idx = LS.find_first(); Idx >= 0; Idx = LS.find_next(Idx)) { - if (!S.LoadRefinements.count(Idx)) + if (!S.Refinements.count(Idx)) + continue; + auto &RefinedPtr = S.Refinements[Idx]; + if (RefinedPtr.empty()) continue; - int RefinedPtr = S.LoadRefinements[Idx]; - if (RefinedPtr == -1 || HasBitSet(LS, RefinedPtr)) + bool rooted = true; + for (auto RefPtr: RefinedPtr) { + if (RefPtr == -1 || HasBitSet(LS, RefPtr)) + continue; + rooted = false; + break; + } + if (rooted) { LS[Idx] = 0; + } } // If the function has GC preserves, figure out whether we need to // add in any extra live values. diff --git a/test/codegen.jl b/test/codegen.jl index b4069e3c77dbf..6b8244b2b439d 100644 --- a/test/codegen.jl +++ b/test/codegen.jl @@ -263,3 +263,35 @@ end @generated f23595(g, args...) = Expr(:call, :g, Expr(:(...), :args)) x23595 = rand(1) @test f23595(Core.arrayref, true, x23595, 1) == x23595[] + +# Issue #22421 +@noinline f22421_1(x) = x[] + 1 +@noinline f22421_2(x) = x[] + 2 +@noinline f22421_3(x, y, z, v) = x[] + y[] + z[] + v +function g22421_1(x, y, b) + # Most likely generates a branch with phi node + if b + z = x + v = f22421_1(y) + else + z = y + v = f22421_2(x) + end + return f22421_3(x, y, z, v) +end +function g22421_2(x, y, b) + # Most likely generates a select + return f22421_3(x, y, b ? x : y, 1) +end + +@test g22421_1(Ref(1), Ref(2), true) === 7 +@test g22421_1(Ref(3), Ref(4), false) === 16 +@test g22421_2(Ref(5), Ref(6), true) === 17 +@test g22421_2(Ref(7), Ref(8), false) === 24 + +if opt_level > 0 + @test !contains(get_llvm(g22421_1, Tuple{Base.RefValue{Int},Base.RefValue{Int},Bool}), + "%gcframe") + @test !contains(get_llvm(g22421_2, Tuple{Base.RefValue{Int},Base.RefValue{Int},Bool}), + "%gcframe") +end diff --git a/test/llvmpasses/gcroots.ll b/test/llvmpasses/gcroots.ll index 26665c70108ec..9e13a67bf3f6c 100644 --- a/test/llvmpasses/gcroots.ll +++ b/test/llvmpasses/gcroots.ll @@ -289,6 +289,27 @@ top: ret %jl_value_t addrspace(10)* %v1 } +define void @refine_select_phi(%jl_value_t addrspace(10)* %x, %jl_value_t addrspace(10)* %y, i1 %b) { +; CHECK-LABEL: @refine_select_phi +; CHECK-NOT: %gcframe +top: + %ptls = call %jl_value_t*** @jl_get_ptls_states() + %s = select i1 %b, %jl_value_t addrspace(10)* %x, %jl_value_t addrspace(10)* %y + br i1 %b, label %L1, label %L2 + +L1: + br label %L3 + +L2: + br label %L3 + +L3: + %p = phi %jl_value_t addrspace(10)* [ %x, %L1 ], [ %y, %L2 ] + call void @one_arg_boxed(%jl_value_t addrspace(10)* %s) + call void @one_arg_boxed(%jl_value_t addrspace(10)* %p) + ret void +} + !0 = !{!"jtbaa"} !1 = !{!"jtbaa_const", !0, i64 0} !2 = !{!1, !1, i64 0, i64 1}