<div dir="ltr">(finally) fixed in r310547 -- sorry this took so long to land<br><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 9, 2017 at 6:58 PM Chandler Carruth via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Sorry, Zach mentioned this to me and I got pulled away before i landed a fix. Fix coming up momentarily!</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Aug 9, 2017 at 5:51 PM Yung, Douglas <<a href="mailto:douglas.yung@sony.com" target="_blank">douglas.yung@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chandler,<br>
<br>
I don't know if you are aware, but this commit seems to have caused the following 11 tests to start failing on one of the bots (<a href="http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/4201" rel="noreferrer" target="_blank">http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/4201</a>):<br>
<br>
LLVM-Unit :: Analysis/Checking/./AnalysisTests.exe/LazyCallGraphTest.InlineAndDeleteFunction<br>
LLVM-Unit :: Analysis/Checking/./AnalysisTests.exe/LazyCallGraphTest.InternalEdgeRemoval<br>
LLVM-Unit :: Analysis/Checking/./AnalysisTests.exe/LazyCallGraphTest.InternalMultiEdgeRemoval<br>
LLVM :: Other/cgscc-devirt-iteration.ll<br>
LLVM :: Other/cgscc-iterate-function-mutation.ll<br>
LLVM :: Transforms/Inline/cgscc-incremental-invalidate.ll<br>
LLVM :: Transforms/Inline/cgscc-invalidate.ll<br>
LLVM :: Transforms/Inline/cgscc-update.ll<br>
LLVM :: Transforms/Inline/frameescape.ll<br>
LLVM :: Transforms/Inline/internal-scc-members.ll<br>
LLVM :: Transforms/Inline/monster_scc.ll<br>
<br>
In each case, the test fails with an assertion failure:<br>
<br>
Assertion failed: G && "Can't have a null graph!", file C:\ps4-buildslave2\llvm-clang-x86_64-expensive-checks-win\llvm\lib\Analysis\LazyCallGraph.cpp, line 276<br>
<br>
Can you take a look?<br>
<br>
Douglas Yung<br>
<br>
> -----Original Message-----<br>
> From: llvm-commits [mailto:<a href="mailto:llvm-commits-bounces@lists.llvm.org" target="_blank">llvm-commits-bounces@lists.llvm.org</a>] On Behalf Of<br>
> Chandler Carruth via llvm-commits<br>
> Sent: Wednesday, August 09, 2017 2:05<br>
> To: <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> Subject: [llvm] r310450 - [LCG] Switch one of the update methods for the<br>
> LazyCallGraph to support<br>
><br>
> Author: chandlerc<br>
> Date: Wed Aug 9 02:05:27 2017<br>
> New Revision: 310450<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=310450&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=310450&view=rev</a><br>
> Log:<br>
> [LCG] Switch one of the update methods for the LazyCallGraph to support<br>
> limited batch updates.<br>
><br>
> Specifically, allow removing multiple reference edges starting from a common<br>
> source node. There are a few constraints that play into supporting this form<br>
> of batching:<br>
><br>
> 1) The way updates occur during the CGSCC walk, about the most we can<br>
> functionally batch together are those with a common source node. This<br>
> also makes the batching simpler to implement, so it seems<br>
> a worthwhile restriction.<br>
> 2) The far and away hottest function for large C++ files I measured<br>
> (generated code for protocol buffers) showed a huge amount of time<br>
> was spent removing ref edges specifically, so it seems worth focusing<br>
> there.<br>
> 3) The algorithm for removing ref edges is very amenable to this<br>
> restricted batching. There are just both API and implementation<br>
> special casing for the non-batch case that gets in the way. Once<br>
> removed, supporting batches is nearly trivial.<br>
><br>
> This does modify the API in an interesting way -- now, we only preserve the<br>
> target RefSCC when the RefSCC structure is unchanged. In the face of any<br>
> splits, we create brand new RefSCC objects. However, all of the users were OK<br>
> with it that I could find. Only the unittest needed interesting updates here.<br>
><br>
> How much does batching these updates help? I instrumented the compiler when<br>
> run over a very large generated source file for a protocol buffer and found<br>
> that the majority of updates are intrinsically updating one function at a<br>
> time. However, nearly 40% of the total ref edges removed are removed as part<br>
> of a batch of removals greater than one, so these are the cases batching can<br>
> help with.<br>
><br>
> When compiling the IR for this file with 'opt' and 'O3', this patch reduces<br>
> the total time by 8-9%.<br>
><br>
> Differential Revision: <a href="https://reviews.llvm.org/D36352" rel="noreferrer" target="_blank">https://reviews.llvm.org/D36352</a><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/Analysis/LazyCallGraph.h<br>
> llvm/trunk/lib/Analysis/CGSCCPassManager.cpp<br>
> llvm/trunk/lib/Analysis/LazyCallGraph.cpp<br>
> llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/Analysis/LazyCallGraph.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/include/llvm/Analysis/LazyCallGraph.h?rev=310450&r1=310449&<br>
> r2=310450&view=diff<br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/Analysis/LazyCallGraph.h (original)<br>
> +++ llvm/trunk/include/llvm/Analysis/LazyCallGraph.h Wed Aug 9 02:05:27<br>
> +++ 2017<br>
> @@ -795,26 +795,25 @@ public:<br>
> /// though, so be careful calling this while iterating over them.<br>
> void removeOutgoingEdge(Node &SourceN, Node &TargetN);<br>
><br>
> - /// Remove a ref edge which is entirely within this RefSCC.<br>
> + /// Remove a list of ref edges which are entirely within this RefSCC.<br>
> ///<br>
> - /// Both the \a SourceN and the \a TargetN must be within this RefSCC.<br>
> - /// Removing such an edge may break cycles that form this RefSCC and thus<br>
> - /// this operation may change the RefSCC graph significantly. In<br>
> + /// Both the \a SourceN and all of the \a TargetNs must be within this<br>
> + /// RefSCC. Removing these edges may break cycles that form this RefSCC<br>
> and<br>
> + /// thus this operation may change the RefSCC graph significantly.<br>
> + In<br>
> /// particular, this operation will re-form new RefSCCs based on the<br>
> /// remaining connectivity of the graph. The following invariants are<br>
> /// guaranteed to hold after calling this method:<br>
> ///<br>
> - /// 1) This RefSCC is still a RefSCC in the graph.<br>
> - /// 2) This RefSCC will be the parent of any new RefSCCs. Thus, this<br>
> RefSCC<br>
> - /// is preserved as the root of any new RefSCC DAG formed.<br>
> - /// 3) No RefSCC other than this RefSCC has its member set changed (this<br>
> is<br>
> + /// 1) If a ref-cycle remains after removal, it leaves this RefSCC intact<br>
> + /// and in the graph. No new RefSCCs are built.<br>
> + /// 2) Otherwise, this RefSCC will be dead after this call and no longer<br>
> in<br>
> + /// the graph or the postorder traversal of the call graph. Any<br>
> iterator<br>
> + /// pointing at this RefSCC will become invalid.<br>
> + /// 3) All newly formed RefSCCs will be returned and the order of the<br>
> + /// RefSCCs returned will be a valid postorder traversal of the new<br>
> + /// RefSCCs.<br>
> + /// 4) No RefSCC other than this RefSCC has its member set changed<br>
> + (this is<br>
> /// inherent in the definition of removing such an edge).<br>
> - /// 4) All of the parent links of the RefSCC graph will be updated to<br>
> - /// reflect the new RefSCC structure.<br>
> - /// 5) All RefSCCs formed out of this RefSCC, excluding this RefSCC, will<br>
> - /// be returned in post-order.<br>
> - /// 6) The order of the RefSCCs in the vector will be a valid postorder<br>
> - /// traversal of the new RefSCCs.<br>
> ///<br>
> /// These invariants are very important to ensure that we can build<br>
> /// optimization pipelines on top of the CGSCC pass manager which @@ -<br>
> 833,11 +832,9 @@ public:<br>
> /// within this RefSCC and edges from this RefSCC to child RefSCCs. Some<br>
> /// effort has been made to minimize the overhead of common cases such as<br>
> /// self-edges and edge removals which result in a spanning tree with no<br>
> - /// more cycles. There are also detailed comments within the<br>
> implementation<br>
> - /// on techniques which could substantially improve this routine's<br>
> - /// efficiency.<br>
> + /// more cycles.<br>
> SmallVector<RefSCC *, 1> removeInternalRefEdge(Node &SourceN,<br>
> - Node &TargetN);<br>
> + ArrayRef<Node *><br>
> + TargetNs);<br>
><br>
> /// A convenience wrapper around the above to handle trivial cases of<br>
> /// inserting a new call edge.<br>
><br>
> Modified: llvm/trunk/lib/Analysis/CGSCCPassManager.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/lib/Analysis/CGSCCPassManager.cpp?rev=310450&r1=310449&r2=3<br>
> 10450&view=diff<br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Analysis/CGSCCPassManager.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/CGSCCPassManager.cpp Wed Aug 9 02:05:27<br>
> +++ 2017<br>
> @@ -459,71 +459,78 @@ LazyCallGraph::SCC &llvm::updateCGAndAna<br>
> VisitRef(*F);<br>
><br>
> // First remove all of the edges that are no longer present in this<br>
> function.<br>
> - // We have to build a list of dead targets first and then remove them as<br>
> the<br>
> - // data structures will all be invalidated by removing them.<br>
> - SmallVector<PointerIntPair<Node *, 1, Edge::Kind>, 4> DeadTargets;<br>
> - for (Edge &E : *N)<br>
> - if (!RetainedEdges.count(&E.getNode()))<br>
> - DeadTargets.push_back({&E.getNode(), E.getKind()});<br>
> - for (auto DeadTarget : DeadTargets) {<br>
> - Node &TargetN = *DeadTarget.getPointer();<br>
> - bool IsCall = DeadTarget.getInt() == Edge::Call;<br>
> - SCC &TargetC = *G.lookupSCC(TargetN);<br>
> - RefSCC &TargetRC = TargetC.getOuterRefSCC();<br>
> -<br>
> - if (&TargetRC != RC) {<br>
> - RC->removeOutgoingEdge(N, TargetN);<br>
> - if (DebugLogging)<br>
> - dbgs() << "Deleting outgoing edge from '" << N << "' to '" << TargetN<br>
> - << "'\n";<br>
> + // The first step makes these edges uniformly ref edges and<br>
> + accumulates them // into a separate data structure so removal doesn't<br>
> invalidate anything.<br>
> + SmallVector<Node *, 4> DeadTargets;<br>
> + for (Edge &E : *N) {<br>
> + if (RetainedEdges.count(&E.getNode()))<br>
> continue;<br>
> - }<br>
> - if (DebugLogging)<br>
> - dbgs() << "Deleting internal " << (IsCall ? "call" : "ref")<br>
> - << " edge from '" << N << "' to '" << TargetN << "'\n";<br>
><br>
> - if (IsCall) {<br>
> + SCC &TargetC = *G.lookupSCC(E.getNode());<br>
> + RefSCC &TargetRC = TargetC.getOuterRefSCC();<br>
> + if (&TargetRC == RC && E.isCall()) {<br>
> if (C != &TargetC) {<br>
> // For separate SCCs this is trivial.<br>
> - RC->switchTrivialInternalEdgeToRef(N, TargetN);<br>
> + RC->switchTrivialInternalEdgeToRef(N, E.getNode());<br>
> } else {<br>
> // Now update the call graph.<br>
> - C = incorporateNewSCCRange(RC->switchInternalEdgeToRef(N, TargetN),<br>
> G,<br>
> - N, C, AM, UR, DebugLogging);<br>
> + C = incorporateNewSCCRange(RC->switchInternalEdgeToRef(N,<br>
> E.getNode()),<br>
> + G, N, C, AM, UR, DebugLogging);<br>
> }<br>
> }<br>
><br>
> - auto NewRefSCCs = RC->removeInternalRefEdge(N, TargetN);<br>
> - if (!NewRefSCCs.empty()) {<br>
> - // Note that we don't bother to invalidate analyses as ref-edge<br>
> - // connectivity is not really observable in any way and is intended<br>
> - // exclusively to be used for ordering of transforms rather than for<br>
> - // analysis conclusions.<br>
> -<br>
> - // The RC worklist is in reverse postorder, so we first enqueue the<br>
> - // current RefSCC as it will remain the parent of all split RefSCCs,<br>
> then<br>
> - // we enqueue the new ones in RPO except for the one which contains the<br>
> - // source node as that is the "bottom" we will continue processing in<br>
> the<br>
> - // bottom-up walk.<br>
> - UR.RCWorklist.insert(RC);<br>
> + // Now that this is ready for actual removal, put it into our list.<br>
> + DeadTargets.push_back(&E.getNode());<br>
> + }<br>
> + // Remove the easy cases quickly and actually pull them out of our list.<br>
> + DeadTargets.erase(<br>
> + llvm::remove_if(DeadTargets,<br>
> + [&](Node *TargetN) {<br>
> + SCC &TargetC = *G.lookupSCC(*TargetN);<br>
> + RefSCC &TargetRC = TargetC.getOuterRefSCC();<br>
> +<br>
> + // We can't trivially remove internal targets, so<br>
> skip<br>
> + // those.<br>
> + if (&TargetRC == RC)<br>
> + return false;<br>
> +<br>
> + RC->removeOutgoingEdge(N, *TargetN);<br>
> + if (DebugLogging)<br>
> + dbgs() << "Deleting outgoing edge from '" << N<br>
> + << "' to '" << TargetN << "'\n";<br>
> + return true;<br>
> + }),<br>
> + DeadTargets.end());<br>
> +<br>
> + // Now do a batch removal of the internal ref edges left.<br>
> + auto NewRefSCCs = RC->removeInternalRefEdge(N, DeadTargets); if<br>
> + (!NewRefSCCs.empty()) {<br>
> + // The old RefSCC is dead, mark it as such.<br>
> + UR.InvalidatedRefSCCs.insert(RC);<br>
> +<br>
> + // Note that we don't bother to invalidate analyses as ref-edge<br>
> + // connectivity is not really observable in any way and is intended<br>
> + // exclusively to be used for ordering of transforms rather than for<br>
> + // analysis conclusions.<br>
> +<br>
> + // Update RC to the "bottom".<br>
> + assert(G.lookupSCC(N) == C && "Changed the SCC when splitting RefSCCs!");<br>
> + RC = &C->getOuterRefSCC();<br>
> + assert(G.lookupRefSCC(N) == RC && "Failed to update current<br>
> + RefSCC!");<br>
> +<br>
> + // The RC worklist is in reverse postorder, so we enqueue the new ones in<br>
> + // RPO except for the one which contains the source node as that is the<br>
> + // "bottom" we will continue processing in the bottom-up walk.<br>
> + assert(NewRefSCCs.front() == RC &&<br>
> + "New current RefSCC not first in the returned list!");<br>
> + for (RefSCC *NewRC :<br>
> + reverse(make_range(std::next(NewRefSCCs.begin()),<br>
> NewRefSCCs.end()))) {<br>
> + assert(NewRC != RC && "Should not encounter the current RefSCC further<br>
> "<br>
> + "in the postorder list of new RefSCCs.");<br>
> + UR.RCWorklist.insert(NewRC);<br>
> if (DebugLogging)<br>
> - dbgs() << "Enqueuing the existing RefSCC in the update worklist: "<br>
> - << *RC << "\n";<br>
> - // Update the RC to the "bottom".<br>
> - assert(G.lookupSCC(N) == C && "Changed the SCC when splitting<br>
> RefSCCs!");<br>
> - RC = &C->getOuterRefSCC();<br>
> - assert(G.lookupRefSCC(N) == RC && "Failed to update current RefSCC!");<br>
> - assert(NewRefSCCs.front() == RC &&<br>
> - "New current RefSCC not first in the returned list!");<br>
> - for (RefSCC *NewRC : reverse(<br>
> - make_range(std::next(NewRefSCCs.begin()), NewRefSCCs.end())))<br>
> {<br>
> - assert(NewRC != RC && "Should not encounter the current RefSCC<br>
> further "<br>
> - "in the postorder list of new RefSCCs.");<br>
> - UR.RCWorklist.insert(NewRC);<br>
> - if (DebugLogging)<br>
> - dbgs() << "Enqueuing a new RefSCC in the update worklist: " <<<br>
> *NewRC<br>
> - << "\n";<br>
> - }<br>
> + dbgs() << "Enqueuing a new RefSCC in the update worklist: " << *NewRC<br>
> + << "\n";<br>
> }<br>
> }<br>
><br>
><br>
> Modified: llvm/trunk/lib/Analysis/LazyCallGraph.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/lib/Analysis/LazyCallGraph.cpp?rev=310450&r1=310449&r2=3104<br>
> 50&view=diff<br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Analysis/LazyCallGraph.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/LazyCallGraph.cpp Wed Aug 9 02:05:27 2017<br>
> @@ -1094,34 +1094,49 @@ void LazyCallGraph::RefSCC::removeOutgoi<br>
> }<br>
><br>
> SmallVector<LazyCallGraph::RefSCC *, 1> -<br>
> LazyCallGraph::RefSCC::removeInternalRefEdge(Node &SourceN, Node &TargetN) {<br>
> - assert(!(*SourceN)[TargetN].isCall() &&<br>
> - "Cannot remove a call edge, it must first be made a ref edge");<br>
> +LazyCallGraph::RefSCC::removeInternalRefEdge(Node &SourceN,<br>
> + ArrayRef<Node *> TargetNs)<br>
> +{<br>
> + // We return a list of the resulting *new* RefSCCs in post-order.<br>
> + SmallVector<RefSCC *, 1> Result;<br>
><br>
> #ifndef NDEBUG<br>
> - // In a debug build, verify the RefSCC is valid to start with and when this<br>
> - // routine finishes.<br>
> + // In a debug build, verify the RefSCC is valid to start with and<br>
> + that either // we return an empty list of result RefSCCs and this<br>
> + RefSCC remains valid, // or we return new RefSCCs and this RefSCC is dead.<br>
> verify();<br>
> - auto VerifyOnExit = make_scope_exit([&]() { verify(); });<br>
> + auto VerifyOnExit = make_scope_exit([&]() {<br>
> + if (Result.empty()) {<br>
> + verify();<br>
> + } else {<br>
> + assert(!G && "A dead RefSCC should have its graph pointer nulled.");<br>
> + assert(SCCs.empty() && "A dead RefSCC should have no SCCs in it.");<br>
> + for (RefSCC *RC : Result)<br>
> + RC->verify();<br>
> + }<br>
> + });<br>
> #endif<br>
><br>
> - // First remove the actual edge.<br>
> - bool Removed = SourceN->removeEdgeInternal(TargetN);<br>
> - (void)Removed;<br>
> - assert(Removed && "Target not in the edge set for this caller?");<br>
> -<br>
> - // We return a list of the resulting *new* RefSCCs in post-order.<br>
> - SmallVector<RefSCC *, 1> Result;<br>
> + // First remove the actual edges.<br>
> + for (Node *TargetN : TargetNs) {<br>
> + assert(!(*SourceN)[*TargetN].isCall() &&<br>
> + "Cannot remove a call edge, it must first be made a ref<br>
> + edge");<br>
> +<br>
> + bool Removed = SourceN->removeEdgeInternal(*TargetN);<br>
> + (void)Removed;<br>
> + assert(Removed && "Target not in the edge set for this caller?");<br>
> + }<br>
><br>
> - // Direct recursion doesn't impact the SCC graph at all.<br>
> - if (&SourceN == &TargetN)<br>
> + // Direct self references don't impact the ref graph at all.<br>
> + if (llvm::all_of(TargetNs,<br>
> + [&](Node *TargetN) { return &SourceN == TargetN; }))<br>
> return Result;<br>
><br>
> - // If this ref edge is within an SCC then there are sufficient other edges<br>
> to<br>
> - // form a cycle without this edge so removing it is a no-op.<br>
> + // If all targets are in the same SCC as the source, because no call<br>
> + edges // were removed there is no RefSCC structure change.<br>
> SCC &SourceC = *G->lookupSCC(SourceN);<br>
> - SCC &TargetC = *G->lookupSCC(TargetN);<br>
> - if (&SourceC == &TargetC)<br>
> + if (llvm::all_of(TargetNs, [&](Node *TargetN) {<br>
> + return G->lookupSCC(*TargetN) == &SourceC;<br>
> + }))<br>
> return Result;<br>
><br>
> // We build somewhat synthetic new RefSCCs by providing a postorder mapping<br>
> @@ -1129,33 +1144,13 @@ LazyCallGraph::RefSCC::removeInternalRef<br>
> // than SCCs because this saves a round-trip through the node->SCC map and<br>
> in<br>
> // the common case, SCCs are small. We will verify that we always give the<br>
> // same number to every node in the SCC such that these are equivalent.<br>
> - const int RootPostOrderNumber = 0;<br>
> - int PostOrderNumber = RootPostOrderNumber + 1;<br>
> + int PostOrderNumber = 0;<br>
> SmallDenseMap<Node *, int> PostOrderMapping;<br>
><br>
> - // Every node in the target SCC can already reach every node in this RefSCC<br>
> - // (by definition). It is the only node we know will stay inside this<br>
> RefSCC.<br>
> - // Everything which transitively reaches Target will also remain in the<br>
> - // RefSCC. We handle this by pre-marking that the nodes in the target SCC<br>
> map<br>
> - // back to the root post order number.<br>
> - //<br>
> - // This also enables us to take a very significant short-cut in the<br>
> standard<br>
> - // Tarjan walk to re-form RefSCCs below: whenever we build an edge that<br>
> - // references the target node, we know that the target node eventually<br>
> - // references all other nodes in our walk. As a consequence, we can detect<br>
> - // and handle participants in that cycle without walking all the edges that<br>
> - // form the connections, and instead by relying on the fundamental<br>
> guarantee<br>
> - // coming into this operation.<br>
> - for (Node &N : TargetC)<br>
> - PostOrderMapping[&N] = RootPostOrderNumber;<br>
> -<br>
> // Reset all the other nodes to prepare for a DFS over them, and add them<br>
> to<br>
> // our worklist.<br>
> SmallVector<Node *, 8> Worklist;<br>
> for (SCC *C : SCCs) {<br>
> - if (C == &TargetC)<br>
> - continue;<br>
> -<br>
> for (Node &N : *C)<br>
> N.DFSNumber = N.LowLink = 0;<br>
><br>
> @@ -1212,26 +1207,6 @@ LazyCallGraph::RefSCC::removeInternalRef<br>
> continue;<br>
> }<br>
> if (ChildN.DFSNumber == -1) {<br>
> - // Check if this edge's target node connects to the deleted edge's<br>
> - // target node. If so, we know that every node connected will end<br>
> up<br>
> - // in this RefSCC, so collapse the entire current stack into the<br>
> root<br>
> - // slot in our SCC numbering. See above for the motivation of<br>
> - // optimizing the target connected nodes in this way.<br>
> - auto PostOrderI = PostOrderMapping.find(&ChildN);<br>
> - if (PostOrderI != PostOrderMapping.end() &&<br>
> - PostOrderI->second == RootPostOrderNumber) {<br>
> - MarkNodeForSCCNumber(*N, RootPostOrderNumber);<br>
> - while (!PendingRefSCCStack.empty())<br>
> - MarkNodeForSCCNumber(*PendingRefSCCStack.pop_back_val(),<br>
> - RootPostOrderNumber);<br>
> - while (!DFSStack.empty())<br>
> - MarkNodeForSCCNumber(*DFSStack.pop_back_val().first,<br>
> - RootPostOrderNumber);<br>
> - // Ensure we break all the way out of the enclosing loop.<br>
> - N = nullptr;<br>
> - break;<br>
> - }<br>
> -<br>
> // If this child isn't currently in this RefSCC, no need to process<br>
> // it.<br>
> ++I;<br>
> @@ -1246,9 +1221,6 @@ LazyCallGraph::RefSCC::removeInternalRef<br>
> N->LowLink = ChildN.LowLink;<br>
> ++I;<br>
> }<br>
> - if (!N)<br>
> - // We short-circuited this node.<br>
> - break;<br>
><br>
> // We've finished processing N and its descendents, put it on our<br>
> pending<br>
> // stack to eventually get merged into a RefSCC.<br>
> @@ -1287,32 +1259,31 @@ LazyCallGraph::RefSCC::removeInternalRef<br>
> assert(PendingRefSCCStack.empty() && "Didn't flush all pending nodes!");<br>
> } while (!Worklist.empty());<br>
><br>
> - // We now have a post-order numbering for RefSCCs and a mapping from each<br>
> - // node in this RefSCC to its final RefSCC. We create each new RefSCC node<br>
> - // (re-using this RefSCC node for the root) and build a radix-sort style<br>
> map<br>
> - // from postorder number to the RefSCC. We then append SCCs to each of<br>
> these<br>
> - // RefSCCs in the order they occured in the original SCCs container.<br>
> - for (int i = 1; i < PostOrderNumber; ++i)<br>
> + // If we only ever needed one post-order number, we reformed a<br>
> + ref-cycle for // every node so the RefSCC remains unchanged.<br>
> + if (PostOrderNumber == 1)<br>
> + return Result;<br>
> +<br>
> + // Otherwise we create a collection of new RefSCC nodes and build //<br>
> + a radix-sort style map from postorder number to these new RefSCCs. We<br>
> + then // append SCCs to each of these RefSCCs in the order they<br>
> + occured in the // original SCCs container.<br>
> + for (int i = 0; i < PostOrderNumber; ++i)<br>
> Result.push_back(G->createRefSCC(*G));<br>
><br>
> // Insert the resulting postorder sequence into the global graph postorder<br>
> - // sequence before the current RefSCC in that sequence. The idea being that<br>
> - // this RefSCC is the target of the reference edge removed, and thus has<br>
> - // a direct or indirect edge to every other RefSCC formed and so must be at<br>
> - // the end of any postorder traversal.<br>
> + // sequence before the current RefSCC in that sequence, and then<br>
> + remove the // current one.<br>
> //<br>
> // FIXME: It'd be nice to change the APIs so that we returned an iterator<br>
> // range over the global postorder sequence and generally use that sequence<br>
> // rather than building a separate result vector here.<br>
> - if (!Result.empty()) {<br>
> - int Idx = G->getRefSCCIndex(*this);<br>
> - G->PostOrderRefSCCs.insert(G->PostOrderRefSCCs.begin() + Idx,<br>
> - Result.begin(), Result.end());<br>
> - for (int i : seq<int>(Idx, G->PostOrderRefSCCs.size()))<br>
> - G->RefSCCIndices[G->PostOrderRefSCCs[i]] = i;<br>
> - assert(G->PostOrderRefSCCs[G->getRefSCCIndex(*this)] == this &&<br>
> - "Failed to update this RefSCC's index after insertion!");<br>
> - }<br>
> + int Idx = G->getRefSCCIndex(*this);<br>
> + G->PostOrderRefSCCs.erase(G->PostOrderRefSCCs.begin() + Idx);<br>
> + G->PostOrderRefSCCs.insert(G->PostOrderRefSCCs.begin() + Idx,<br>
> Result.begin(),<br>
> + Result.end()); for (int i : seq<int>(Idx,<br>
> + G->PostOrderRefSCCs.size()))<br>
> + G->RefSCCIndices[G->PostOrderRefSCCs[i]] = i;<br>
><br>
> for (SCC *C : SCCs) {<br>
> auto PostOrderI = PostOrderMapping.find(&*C->begin());<br>
> @@ -1324,33 +1295,19 @@ LazyCallGraph::RefSCC::removeInternalRef<br>
> assert(PostOrderMapping.find(&N)->second == SCCNumber &&<br>
> "Cannot have different numbers for nodes in the same SCC!");<br>
> #endif<br>
> - if (SCCNumber == 0)<br>
> - // The root node is handled separately by removing the SCCs.<br>
> - continue;<br>
><br>
> - RefSCC &RC = *Result[SCCNumber - 1];<br>
> + RefSCC &RC = *Result[SCCNumber];<br>
> int SCCIndex = RC.SCCs.size();<br>
> RC.SCCs.push_back(C);<br>
> RC.SCCIndices[C] = SCCIndex;<br>
> C->OuterRefSCC = &RC;<br>
> }<br>
><br>
> - // Now erase all but the root's SCCs.<br>
> - SCCs.erase(remove_if(SCCs,<br>
> - [&](SCC *C) {<br>
> - return PostOrderMapping.lookup(&*C->begin()) !=<br>
> - RootPostOrderNumber;<br>
> - }),<br>
> - SCCs.end());<br>
> + // Now that we've moved things into the new RefSCCs, clear out our<br>
> + current // one.<br>
> + G = nullptr;<br>
> + SCCs.clear();<br>
> SCCIndices.clear();<br>
> - for (int i = 0, Size = SCCs.size(); i < Size; ++i)<br>
> - SCCIndices[SCCs[i]] = i;<br>
> -<br>
> -#ifndef NDEBUG<br>
> - // Verify all of the new RefSCCs.<br>
> - for (RefSCC *RC : Result)<br>
> - RC->verify();<br>
> -#endif<br>
><br>
> // Return the new list of SCCs.<br>
> return Result;<br>
><br>
> Modified: llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-</a><br>
> project/llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp?rev=310450&r1=3104<br>
> 49&r2=310450&view=diff<br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp (original)<br>
> +++ llvm/trunk/unittests/Analysis/LazyCallGraphTest.cpp Wed Aug 9<br>
> +++ 02:05:27 2017<br>
> @@ -1166,20 +1166,21 @@ TEST(LazyCallGraphTest, InlineAndDeleteF<br>
> LazyCallGraph::SCC &NewDC = *NewCs.begin();<br>
> EXPECT_EQ(&NewDC, CG.lookupSCC(D1));<br>
> EXPECT_EQ(&NewDC, CG.lookupSCC(D3));<br>
> - auto NewRCs = DRC.removeInternalRefEdge(D1, D2);<br>
> - EXPECT_EQ(&DRC, CG.lookupRefSCC(D2));<br>
> - EXPECT_EQ(NewRCs.end(), std::next(NewRCs.begin()));<br>
> - LazyCallGraph::RefSCC &NewDRC = **NewRCs.begin();<br>
> + auto NewRCs = DRC.removeInternalRefEdge(D1, {&D2}); ASSERT_EQ(2u,<br>
> + NewRCs.size()); LazyCallGraph::RefSCC &NewDRC = *NewRCs[0];<br>
> EXPECT_EQ(&NewDRC, CG.lookupRefSCC(D1));<br>
> EXPECT_EQ(&NewDRC, CG.lookupRefSCC(D3));<br>
> - EXPECT_FALSE(NewDRC.isParentOf(DRC));<br>
> - EXPECT_TRUE(CRC.isParentOf(DRC));<br>
> + LazyCallGraph::RefSCC &D2RC = *NewRCs[1]; EXPECT_EQ(&D2RC,<br>
> + CG.lookupRefSCC(D2)); EXPECT_FALSE(NewDRC.isParentOf(D2RC));<br>
> + EXPECT_TRUE(CRC.isParentOf(D2RC));<br>
> EXPECT_TRUE(CRC.isParentOf(NewDRC));<br>
> - EXPECT_TRUE(DRC.isParentOf(NewDRC));<br>
> + EXPECT_TRUE(D2RC.isParentOf(NewDRC));<br>
> CRC.removeOutgoingEdge(C1, D2);<br>
> - EXPECT_FALSE(CRC.isParentOf(DRC));<br>
> + EXPECT_FALSE(CRC.isParentOf(D2RC));<br>
> EXPECT_TRUE(CRC.isParentOf(NewDRC));<br>
> - EXPECT_TRUE(DRC.isParentOf(NewDRC));<br>
> + EXPECT_TRUE(D2RC.isParentOf(NewDRC));<br>
><br>
> // Now that we've updated the call graph, D2 is dead, so remove it.<br>
> CG.removeDeadFunction(D2F);<br>
> @@ -1340,7 +1341,7 @@ TEST(LazyCallGraphTest, InternalEdgeRemo<br>
> // Remove the edge from b -> a, which should leave the 3 functions still in<br>
> // a single connected component because of a -> b -> c -> a.<br>
> SmallVector<LazyCallGraph::RefSCC *, 1> NewRCs =<br>
> - RC.removeInternalRefEdge(B, A);<br>
> + RC.removeInternalRefEdge(B, {&A});<br>
> EXPECT_EQ(0u, NewRCs.size());<br>
> EXPECT_EQ(&RC, CG.lookupRefSCC(A));<br>
> EXPECT_EQ(&RC, CG.lookupRefSCC(B));<br>
> @@ -1350,24 +1351,94 @@ TEST(LazyCallGraphTest, InternalEdgeRemo<br>
> EXPECT_EQ(&RC, &*J);<br>
> EXPECT_EQ(E, std::next(J));<br>
><br>
> + // Increment I before we actually mutate the structure so that it<br>
> + remains // a valid iterator.<br>
> + ++I;<br>
> +<br>
> // Remove the edge from c -> a, which should leave 'a' in the original<br>
> RefSCC<br>
> // and form a new RefSCC for 'b' and 'c'.<br>
> - NewRCs = RC.removeInternalRefEdge(C, A);<br>
> - EXPECT_EQ(1u, NewRCs.size());<br>
> - EXPECT_EQ(&RC, CG.lookupRefSCC(A));<br>
> - EXPECT_EQ(1, std::distance(RC.begin(), RC.end()));<br>
> - LazyCallGraph::RefSCC &RC2 = *CG.lookupRefSCC(B);<br>
> - EXPECT_EQ(&RC2, CG.lookupRefSCC(C));<br>
> - EXPECT_EQ(&RC2, NewRCs[0]);<br>
> + NewRCs = RC.removeInternalRefEdge(C, {&A}); ASSERT_EQ(2u,<br>
> + NewRCs.size()); LazyCallGraph::RefSCC &BCRC = *NewRCs[0];<br>
> + LazyCallGraph::RefSCC &ARC = *NewRCs[1]; EXPECT_EQ(&ARC,<br>
> + CG.lookupRefSCC(A)); EXPECT_EQ(1, std::distance(ARC.begin(),<br>
> + ARC.end())); EXPECT_EQ(&BCRC, CG.lookupRefSCC(B)); EXPECT_EQ(&BCRC,<br>
> + CG.lookupRefSCC(C));<br>
> J = CG.postorder_ref_scc_begin();<br>
> EXPECT_NE(I, J);<br>
> - EXPECT_EQ(&RC2, &*J);<br>
> + EXPECT_EQ(&BCRC, &*J);<br>
> + ++J;<br>
> + EXPECT_NE(I, J);<br>
> + EXPECT_EQ(&ARC, &*J);<br>
> ++J;<br>
> EXPECT_EQ(I, J);<br>
> - EXPECT_EQ(&RC, &*J);<br>
> + EXPECT_EQ(E, J);<br>
> +}<br>
> +<br>
> +TEST(LazyCallGraphTest, InternalMultiEdgeRemoval) {<br>
> + LLVMContext Context;<br>
> + // A nice fully connected (including self-edges) RefSCC.<br>
> + std::unique_ptr<Module> M = parseAssembly(<br>
> + Context, "define void @a(i8** %ptr) {\n"<br>
> + "entry:\n"<br>
> + " store i8* bitcast (void(i8**)* @a to i8*), i8** %ptr\n"<br>
> + " store i8* bitcast (void(i8**)* @b to i8*), i8** %ptr\n"<br>
> + " store i8* bitcast (void(i8**)* @c to i8*), i8** %ptr\n"<br>
> + " ret void\n"<br>
> + "}\n"<br>
> + "define void @b(i8** %ptr) {\n"<br>
> + "entry:\n"<br>
> + " store i8* bitcast (void(i8**)* @a to i8*), i8** %ptr\n"<br>
> + " store i8* bitcast (void(i8**)* @b to i8*), i8** %ptr\n"<br>
> + " store i8* bitcast (void(i8**)* @c to i8*), i8** %ptr\n"<br>
> + " ret void\n"<br>
> + "}\n"<br>
> + "define void @c(i8** %ptr) {\n"<br>
> + "entry:\n"<br>
> + " store i8* bitcast (void(i8**)* @a to i8*), i8** %ptr\n"<br>
> + " store i8* bitcast (void(i8**)* @b to i8*), i8** %ptr\n"<br>
> + " store i8* bitcast (void(i8**)* @c to i8*), i8** %ptr\n"<br>
> + " ret void\n"<br>
> + "}\n");<br>
> + LazyCallGraph CG = buildCG(*M);<br>
> +<br>
> + // Force the graph to be fully expanded.<br>
> + CG.buildRefSCCs();<br>
> + auto I = CG.postorder_ref_scc_begin(), E =<br>
> + CG.postorder_ref_scc_end(); LazyCallGraph::RefSCC &RC = *I;<br>
> + EXPECT_EQ(E, std::next(I));<br>
> +<br>
> + LazyCallGraph::Node &A = *CG.lookup(lookupFunction(*M, "a"));<br>
> + LazyCallGraph::Node &B = *CG.lookup(lookupFunction(*M, "b"));<br>
> + LazyCallGraph::Node &C = *CG.lookup(lookupFunction(*M, "c"));<br>
> + EXPECT_EQ(&RC, CG.lookupRefSCC(A)); EXPECT_EQ(&RC,<br>
> + CG.lookupRefSCC(B)); EXPECT_EQ(&RC, CG.lookupRefSCC(C));<br>
> +<br>
> + // Increment I before we actually mutate the structure so that it<br>
> + remains // a valid iterator.<br>
> ++I;<br>
> - EXPECT_EQ(E, I);<br>
> +<br>
> + // Remove the edges from b -> a and b -> c, leaving b in its own RefSCC.<br>
> + SmallVector<LazyCallGraph::RefSCC *, 1> NewRCs =<br>
> + RC.removeInternalRefEdge(B, {&A, &C});<br>
> +<br>
> + ASSERT_EQ(2u, NewRCs.size());<br>
> + LazyCallGraph::RefSCC &BRC = *NewRCs[0]; LazyCallGraph::RefSCC &ACRC<br>
> + = *NewRCs[1]; EXPECT_EQ(&BRC, CG.lookupRefSCC(B)); EXPECT_EQ(1,<br>
> + std::distance(BRC.begin(), BRC.end())); EXPECT_EQ(&ACRC,<br>
> + CG.lookupRefSCC(A)); EXPECT_EQ(&ACRC, CG.lookupRefSCC(C)); auto J =<br>
> + CG.postorder_ref_scc_begin(); EXPECT_NE(I, J); EXPECT_EQ(&BRC, &*J);<br>
> ++J;<br>
> + EXPECT_NE(I, J);<br>
> + EXPECT_EQ(&ACRC, &*J);<br>
> + ++J;<br>
> + EXPECT_EQ(I, J);<br>
> EXPECT_EQ(E, J);<br>
> }<br>
><br>
> @@ -1420,7 +1491,7 @@ TEST(LazyCallGraphTest, InternalNoOpEdge<br>
><br>
> // Remove the edge from a -> c which doesn't change anything.<br>
> SmallVector<LazyCallGraph::RefSCC *, 1> NewRCs =<br>
> - RC.removeInternalRefEdge(AN, CN);<br>
> + RC.removeInternalRefEdge(AN, {&CN});<br>
> EXPECT_EQ(0u, NewRCs.size());<br>
> EXPECT_EQ(&RC, CG.lookupRefSCC(AN));<br>
> EXPECT_EQ(&RC, CG.lookupRefSCC(BN));<br>
> @@ -1435,8 +1506,8 @@ TEST(LazyCallGraphTest, InternalNoOpEdge<br>
><br>
> // Remove the edge from b -> a and c -> b; again this doesn't change<br>
> // anything.<br>
> - NewRCs = RC.removeInternalRefEdge(BN, AN);<br>
> - NewRCs = RC.removeInternalRefEdge(CN, BN);<br>
> + NewRCs = RC.removeInternalRefEdge(BN, {&AN}); NewRCs =<br>
> + RC.removeInternalRefEdge(CN, {&BN});<br>
> EXPECT_EQ(0u, NewRCs.size());<br>
> EXPECT_EQ(&RC, CG.lookupRefSCC(AN));<br>
> EXPECT_EQ(&RC, CG.lookupRefSCC(BN));<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>