[llvm] r204857 - Simplify PBQP graph removeAdjEdgeId implementation.

Lang Hames lhames at gmail.com
Sat Mar 29 16:26:51 PDT 2014


It appears we do not optimise the conditionals away at the moment. I might
have a look in to how difficult it is.

- Lang.


On Thu, Mar 27, 2014 at 4:50 PM, David Blaikie <dblaikie at gmail.com> wrote:

> On Wed, Mar 26, 2014 at 3:16 PM, Lang Hames <lhames at gmail.com> wrote:
> > Correct on all points. Well - I made the last cosmetic fixes - I think
> it's
> > your turn to make these. ;)
>
> r204983
>
> > It's about time you got your name on the blame list for this thing
> anyway.
>
> And now I am eternally tainted.
>
> As an aside, you could write setAdjEdgeIdx like this:
>
> ThisEdgeAdjIdxs[NId == NIds[0]] = NewIdx;
>
> Skipping the conditionals. Not sure if LLVM's smart enough to do that
> optimization for you - I hope so. (& guess if it does, you prefer the
> way it's currently written, for legibility - which I'm OK with)
>
> I've seen this sort of "boolean test as index into two element array"
> as semi-idiomatic in various tree structures, for example.
>
> - David
>
> > On Wed, Mar 26, 2014 at 2:55 PM, David Blaikie <dblaikie at gmail.com>
> wrote:
> >>
> >> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140329/4372b5af/attachment.html>


More information about the llvm-commits mailing list