<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>