<div dir="ltr">Correct on all points. Well - I made the last cosmetic fixes - I think it's your turn to make these. ;)<div><br></div><div>It's about time you got your name on the blame list for this thing anyway.<div>
<br><div><font face="arial, sans-serif">- Lang.<br></font><div class="gmail_extra"><br></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Mar 26, 2014 at 2:55 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Wed, Mar 26, 2014 at 2:21 PM, Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br>

> Author: lhames<br>
> Date: Wed Mar 26 16:21:53 2014<br>
> New Revision: 204857<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=204857&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=204857&view=rev</a><br>
> Log:<br>
> Simplify PBQP graph removeAdjEdgeId implementation.<br>
><br>
> Modified:<br>
>     llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h<br>
><br>
> Modified: llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h?rev=204857&r1=204856&r2=204857&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h?rev=204857&r1=204856&r2=204857&view=diff</a><br>

> ==============================================================================<br>
> --- llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h (original)<br>
> +++ llvm/trunk/include/llvm/CodeGen/PBQP/Graph.h Wed Mar 26 16:21:53 2014<br>
> @@ -67,16 +67,16 @@ namespace PBQP {<br>
>          return Idx;<br>
>        }<br>
><br>
> -      // If a swap is performed, returns the new EdgeId that must be<br>
> -      // updated, otherwise returns invalidEdgeId().<br>
> -      EdgeId removeAdjEdgeId(AdjEdgeIdx Idx) {<br>
> -        EdgeId EIdToUpdate = Graph::invalidEdgeId();<br>
> -        if (Idx < AdjEdgeIds.size() - 1) {<br>
> -          std::swap(AdjEdgeIds[Idx], AdjEdgeIds.back());<br>
> -          EIdToUpdate = AdjEdgeIds[Idx];<br>
> -        }<br>
> +      void removeAdjEdgeId(Graph &G, NodeId ThisNId, AdjEdgeIdx Idx) {<br>
> +        // Swap-and-pop for fast removal.<br>
> +        //   1) Update the adj index of the edge currently at back().<br>
> +        //   2) Swap Edge at Idx with back().<br>
> +        //   3) pop_back()<br>
> +        // If Idx == size() - 1 then the updateAdjEdgeIdx and swap are<br>
> +        // redundant, but both operations are cheap.<br>
> +        G.getEdge(AdjEdgeIds.back()).updateAdjEdgeIdx(ThisNId, Idx);<br>
<br>
</div></div>Is there more to this "updateAdjEdgeIdx" function or is it just a<br>
trivial (indexed/mapped) setter? "update" rather than "set" makes me<br>
wonder what work it's doing...<br>
<div class=""><br>
> +        std::swap(AdjEdgeIds[Idx], AdjEdgeIds.back());<br>
<br>
</div>Since you're about to pop the back anyway, this could just be<br>
AdjEdgeIds[Idx] = AdjEdgeIds.back(); right? (if they were movable, you<br>
could add a std::move() around the RHS, but they're just indexes,<br>
right?)<br>
<br>
(& comment 2 would be "move back() down to fill the hole at Idx" or<br>
something like that)<br>
<div class="HOEnZb"><div class="h5"><br>
>          AdjEdgeIds.pop_back();<br>
> -        return EIdToUpdate;<br>
>        }<br>
><br>
>        const AdjEdgeList& getAdjEdgeIds() const { return AdjEdgeIds; }<br>
> @@ -138,9 +138,7 @@ namespace PBQP {<br>
>          assert(ThisEdgeAdjIdxs[NIdx] != NodeEntry::getInvalidAdjEdgeIdx() &&<br>
>                 "Edge not connected to NIds[NIdx].");<br>
>          NodeEntry &N = G.getNode(NIds[NIdx]);<br>
> -        EdgeId EIdToUpdate = N.removeAdjEdgeId(ThisEdgeAdjIdxs[NIdx]);<br>
> -        if (EIdToUpdate != Graph::invalidEdgeId())<br>
> -          G.getEdge(EIdToUpdate).updateAdjEdgeIdx(NIds[NIdx], ThisEdgeAdjIdxs[NIdx]);<br>
> +        N.removeAdjEdgeId(G, NIds[NIdx], ThisEdgeAdjIdxs[NIdx]);<br>
>          ThisEdgeAdjIdxs[NIdx] = NodeEntry::getInvalidAdjEdgeIdx();<br>
>        }<br>
><br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div></div></div></div></div>