From 8b190f84969b15a772ef2ba5a5989389cbece426 Mon Sep 17 00:00:00 2001 From: Philip Reames Date: Mon, 13 Apr 2015 18:07:21 +0000 Subject: [PATCH] [RewriteStatepointsForGC] Fix a latent bug in normalization for invoke statepoint [NFC] Since we're restructuring the CFG, we also need to make sure to update the analsis passes. While I'm touching the code, I dedicided to restructure it a bit. The code involved here was very confusing. This change moves the normalization to essentially being a pre-pass before the main insertion work and updates a few comments to actually say what is happening and *why*. The restructuring should be covered by existing tests. I couldn't easily see how to create a test for the invalidation bug. Suggestions welcome. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@234769 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../Scalar/RewriteStatepointsForGC.cpp | 77 ++++++++++--------- 1 file changed, 40 insertions(+), 37 deletions(-) diff --git a/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp b/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp index b4752243b6a..47d6ffb3064 100644 --- a/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp +++ b/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp @@ -997,27 +997,32 @@ static void recomputeLiveInValues( } } -// Normalize basic block to make it ready to be target of invoke statepoint. -// It means spliting it to have single predecessor. Return newly created BB -// ready to be successor of invoke statepoint. -static BasicBlock *normalizeBBForInvokeSafepoint(BasicBlock *BB, +// When inserting gc.relocate calls, we need to ensure there are no uses +// of the original value between the gc.statepoint and the gc.relocate call. +// One case which can arise is a phi node starting one of the successor blocks. +// We also need to be able to insert the gc.relocates only on the path which +// goes through the statepoint. We might need to split an edge to make this +// possible. +static BasicBlock *normalizeForInvokeSafepoint(BasicBlock *BB, BasicBlock *InvokeParent, Pass *P) { - BasicBlock *ret = BB; + DominatorTree *DT = nullptr; + if (auto *DTP = P->getAnalysisIfAvailable()) + DT = &DTP->getDomTree(); + BasicBlock *Ret = BB; if (!BB->getUniquePredecessor()) { - ret = SplitBlockPredecessors(BB, InvokeParent, ""); + Ret = SplitBlockPredecessors(BB, InvokeParent, "", nullptr, DT); } - // Another requirement for such basic blocks is to not have any phi nodes. - // Since we just ensured that new BB will have single predecessor, - // all phi nodes in it will have one value. Here it would be naturall place - // to - // remove them all. But we can not do this because we are risking to remove - // one of the values stored in liveset of another statepoint. We will do it - // later after placing all safepoints. + // Now that 'ret' has unique predecessor we can safely remove all phi nodes + // from it + FoldSingleEntryPHINodes(Ret); + assert(!isa(Ret->begin())); - return ret; + // At this point, we can safely insert a gc.relocate as the first instruction + // in Ret if needed. + return Ret; } static int find_index(ArrayRef livevec, Value *val) { @@ -1203,8 +1208,10 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */ token = invoke; // Generate gc relocates in exceptional path - BasicBlock *unwindBlock = normalizeBBForInvokeSafepoint( - toReplace->getUnwindDest(), invoke->getParent(), P); + BasicBlock *unwindBlock = toReplace->getUnwindDest(); + assert(!isa(unwindBlock->begin()) && + unwindBlock->getUniquePredecessor() && + "can't safely insert in this block!"); Instruction *IP = &*(unwindBlock->getFirstInsertionPt()); Builder.SetInsertPoint(IP); @@ -1224,8 +1231,10 @@ makeStatepointExplicitImpl(const CallSite &CS, /* to replace */ exceptional_token, Builder); // Generate gc relocates and returns for normal block - BasicBlock *normalDest = normalizeBBForInvokeSafepoint( - toReplace->getNormalDest(), invoke->getParent(), P); + BasicBlock *normalDest = toReplace->getNormalDest(); + assert(!isa(normalDest->begin()) && + normalDest->getUniquePredecessor() && + "can't safely insert in this block!"); IP = &*(normalDest->getFirstInsertionPt()); Builder.SetInsertPoint(IP); @@ -1722,6 +1731,19 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P, } #endif + // When inserting gc.relocates for invokes, we need to be able to insert at + // the top of the successor blocks. See the comment on + // normalForInvokeSafepoint on exactly what is needed. Note that this step + // may restructure the CFG. + for (CallSite CS : toUpdate) + if (CS.isInvoke()) { + InvokeInst *invoke = cast(CS.getInstruction()); + normalizeForInvokeSafepoint(invoke->getNormalDest(), + invoke->getParent(), P); + normalizeForInvokeSafepoint(invoke->getUnwindDest(), + invoke->getParent(), P); + } + // A list of dummy calls added to the IR to keep various values obviously // live in the IR. We'll remove all of these when done. SmallVector holders; @@ -1848,25 +1870,6 @@ static bool insertParsePoints(Function &F, DominatorTree &DT, Pass *P, } toUpdate.clear(); // prevent accident use of invalid CallSites - // In case if we inserted relocates in a different basic block than the - // original safepoint (this can happen for invokes). We need to be sure that - // original values were not used in any of the phi nodes at the - // beginning of basic block containing them. Because we know that all such - // blocks will have single predecessor we can safely assume that all phi - // nodes have single entry (because of normalizeBBForInvokeSafepoint). - // Just remove them all here. - for (size_t i = 0; i < records.size(); i++) { - Instruction *I = records[i].StatepointToken; - - if (InvokeInst *invoke = dyn_cast(I)) { - FoldSingleEntryPHINodes(invoke->getNormalDest()); - assert(!isa(invoke->getNormalDest()->begin())); - - FoldSingleEntryPHINodes(invoke->getUnwindDest()); - assert(!isa(invoke->getUnwindDest()->begin())); - } - } - // Do all the fixups of the original live variables to their relocated selves SmallVector live; for (size_t i = 0; i < records.size(); i++) { -- 2.34.1