[llvm] r204857 - Simplify PBQP graph removeAdjEdgeId implementation.

[David Blaikie]

On Wed, Mar 26, 2014 at 2:21 PM, Lang Hames <lhames at gmail.com> wrote:
> Author: lhames
> Date: Wed Mar 26 16:21:53 2014
> New Revision: 204857
>
> URL: http://llvm.org/viewvc/llvm-project?rev=204857&view=rev
> Log:
> Simplify PBQP graph removeAdjEdgeId implementation.
>
> Modified:
>     llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h
>
> Modified: llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h?rev=204857&r1=204856&r2=204857&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h Wed Mar 26 16:21:53 2014
> @@ -67,16 +67,16 @@ namespace PBQP {
>          return Idx;
>        }
>
> -      // If a swap is performed, returns the new EdgeId that must be
> -      // updated, otherwise returns invalidEdgeId().
> -      EdgeId removeAdjEdgeId(AdjEdgeIdx Idx) {
> -        EdgeId EIdToUpdate = Graph::invalidEdgeId();
> -        if (Idx < AdjEdgeIds.size() - 1) {
> -          std::swap(AdjEdgeIds[Idx], AdjEdgeIds.back());
> -          EIdToUpdate = AdjEdgeIds[Idx];
> -        }
> +      void removeAdjEdgeId(Graph &G, NodeId ThisNId, AdjEdgeIdx Idx) {
> +        // Swap-and-pop for fast removal.
> +        //   1) Update the adj index of the edge currently at back().
> +        //   2) Swap Edge at Idx with back().
> +        //   3) pop_back()
> +        // If Idx == size() - 1 then the updateAdjEdgeIdx and swap are
> +        // redundant, but both operations are cheap.
> +        G.getEdge(AdjEdgeIds.back()).updateAdjEdgeIdx(ThisNId, Idx);

Is there more to this "updateAdjEdgeIdx" function or is it just a
trivial (indexed/mapped) setter? "update" rather than "set" makes me
wonder what work it's doing...

> +        std::swap(AdjEdgeIds[Idx], AdjEdgeIds.back());

Since you're about to pop the back anyway, this could just be
AdjEdgeIds[Idx] = AdjEdgeIds.back(); right? (if they were movable, you
could add a std::move() around the RHS, but they're just indexes,
right?)

(& comment 2 would be "move back() down to fill the hole at Idx" or
something like that)

>          AdjEdgeIds.pop_back();
> -        return EIdToUpdate;
>        }
>
>        const AdjEdgeList& getAdjEdgeIds() const { return AdjEdgeIds; }
> @@ -138,9 +138,7 @@ namespace PBQP {
>          assert(ThisEdgeAdjIdxs[NIdx] != NodeEntry::getInvalidAdjEdgeIdx() &&
>                 "Edge not connected to NIds[NIdx].");
>          NodeEntry &N = G.getNode(NIds[NIdx]);
> -        EdgeId EIdToUpdate = N.removeAdjEdgeId(ThisEdgeAdjIdxs[NIdx]);
> -        if (EIdToUpdate != Graph::invalidEdgeId())
> -          G.getEdge(EIdToUpdate).updateAdjEdgeIdx(NIds[NIdx], ThisEdgeAdjIdxs[NIdx]);
> +        N.removeAdjEdgeId(G, NIds[NIdx], ThisEdgeAdjIdxs[NIdx]);
>          ThisEdgeAdjIdxs[NIdx] = NodeEntry::getInvalidAdjEdgeIdx();
>        }
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits