<div dir="ltr"><div>It appears we do not optimise the conditionals away at the moment. I might have a look in to how difficult it is.</div><div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">
On Thu, Mar 27, 2014 at 4:50 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="">On Wed, Mar 26, 2014 at 3:16 PM, Lang Hames <<a href="mailto:lhames@gmail.com">lhames@gmail.com</a>> wrote:<br>
> Correct on all points. Well - I made the last cosmetic fixes - I think it's<br>
> your turn to make these. ;)<br>
<br>
</div>r204983<br>
<div class=""><br>
> It's about time you got your name on the blame list for this thing anyway.<br>
<br>
</div>And now I am eternally tainted.<br>
<br>
As an aside, you could write setAdjEdgeIdx like this:<br>
<br>
ThisEdgeAdjIdxs[NId == NIds[0]] = NewIdx;<br>
<br>
Skipping the conditionals. Not sure if LLVM's smart enough to do that<br>
optimization for you - I hope so. (& guess if it does, you prefer the<br>
way it's currently written, for legibility - which I'm OK with)<br>
<br>
I've seen this sort of "boolean test as index into two element array"<br>
as semi-idiomatic in various tree structures, for example.<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> On Wed, Mar 26, 2014 at 2:55 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>><br>
>> 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:<br>
>> > <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>
>> > ==============================================================================<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<br>
>> > 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>
>> 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>
>><br>
>> > + std::swap(AdjEdgeIds[Idx], AdjEdgeIds.back());<br>
>><br>
>> 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>
>><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] !=<br>
>> > 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],<br>
>> > 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>
><br>
><br>
</div></div></blockquote></div><br></div>