<div dir="ltr">Hi David,<div><br></div><div>Committed as r217682.</div><div><br></div><div>Cheers!</div><div><br></div><div>James</div></div><div class="gmail_extra"><br><div class="gmail_quote">On 26 August 2014 18:25, 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">Right - thanks for the ping & sorry for the delay. (& thanks for<br>
having a go/taking my feedback)<br>
<br>
As I was suggesting in my previous email, this:<br>
<br>
+ for (auto &I : AllChains) {<br>
+ for (auto &J : AllChains) {<br>
+ Chain *PI = I.get();<br>
+ Chain *PJ = J.get();<br>
+ if (PI != PJ && PI->rangeOverlapsWith(PJ))<br>
+ EC.unionSets(PI, PJ);<br>
<br>
could be simplified to:<br>
<br>
+ for (auto &I : AllChains) {<br>
+ for (auto &J : AllChains) {<br>
+ if (I != J && I->rangeOverlapsWith(*J))<br>
+ EC.unionSets(*I, *J);<br>
<br>
if rangeOverlapsWith and unionSets were first changed to take<br>
references instead of pointers. (I've taken to doing such ref-ifying<br>
changes as a separate preliminary commit, thus making the<br>
unique_ptr-ification change smaller/simpler - in this case if you did<br>
that preliminary API change, the only thing that would change in that<br>
loop is that the "auto *I" (and J) would become "auto &I" and<br>
everything else is identical)<br>
<br>
This:<br>
<br>
Chain *G = new Chain(MI, Idx, getColor(DestReg));<br>
ActiveChains[DestReg] = G;<br>
- AllChains.insert(G);<br>
+ AllChains.insert(std::unique_ptr<Chain>(G));<br>
<br>
(and similar code in the next hunk) I would rewrite as:<br>
<br>
auto G = llvm::make_unique(MI, Idx, getColor(DestReg));<br>
ActiveChains[DestReg] = G.get();<br>
AllChains.insert(std::move(G));<br>
<br>
To make it iron-clad that ownership is taken and passed safely (the<br>
raw pointer/raw new will hopefully one day be a warning flag to look<br>
carefully at code - so it's helpful to avoid using it at all where<br>
practical)<br>
<span class="HOEnZb"><font color="#888888"><br>
- David<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Tue, Aug 26, 2014 at 6:09 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk">james@jamesmolloy.co.uk</a>> wrote:<br>
> Hi David,<br>
><br>
> Do you have any comments on this change? I'm loath to commit it without your<br>
> say-so.<br>
><br>
> Cheers,<br>
> James<br>
><br>
><br>
> On 11 August 2014 16:29, James Molloy <<a href="mailto:james@jamesmolloy.co.uk">james@jamesmolloy.co.uk</a>> wrote:<br>
>><br>
>> Hi David,<br>
>><br>
>> OK, having taken a second look the unique_ptr idiom doesn't look so bad.<br>
>> Attached is what I've come up with - given I'm new to the idiom, would you<br>
>> mind taking a look before I commit?<br>
>><br>
>> Cheers,<br>
>><br>
>> James<br>
>><br>
>><br>
>> On 8 August 2014 17:02, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>><br>
>>> On Fri, Aug 8, 2014 at 8:39 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk">james@jamesmolloy.co.uk</a>><br>
>>> wrote:<br>
>>> > Hi David,<br>
>>> ><br>
>>> > Partly this may be to do with my unfamiliarity with the unique_ptr<br>
>>> > idiom,<br>
>>> > and may be using it incorrectly. We're talking about pointers to "class<br>
>>> > Chain", which reside in two containers:<br>
>>> ><br>
>>> > std::set<Chain*> AllChains;<br>
>>> > std::map<unsigned, Chain*> ActiveChains;<br>
>>> ><br>
>>> > AllChains obviously contains all the chains, and ActiveChains may<br>
>>> > contain a<br>
>>> > subset of AllChains (as values); there may be duplicates too.<br>
>>><br>
>>> There may be duplicates in ActiveChains, that is? (AllChains is a set,<br>
>>> so it's safe/unique)<br>
>>><br>
>>> So ActiveChains both contains not all chains and duplicate chains - so<br>
>>> it's not directly a candidate for single ownership, at least.<br>
>>><br>
>>> > To make this<br>
>>> > work, I'd need to change AllChains to be a<br>
>>> > std::set<std::unique_ptr<Chain>>,<br>
>>> > and would have to use "::get()" to get the unmanaged version of the<br>
>>> > pointer<br>
>>> > before it could be passed into ActiveChains, as I'm effectively<br>
>>> > duplicating<br>
>>> > the pointer. This seems to hack around the intended semantics<br>
>>> > (shared_ptr<br>
>>> > might be better here?).<br>
>>><br>
>>> I don't think this is hacking around the intended semantics,<br>
>>> necessarily. The names (ActiveChains and AllChains) actually give a<br>
>>> pretty good indicator to me, the unknowing reader, that ActiveChains<br>
>>> is a subset of AllChains (I wouldn't've guessed that ActiveChains<br>
>>> might have duplicates, but that's not too important) and seeing<br>
>>> AllChains have unique_ptr and ActiveChains have raw (non-owning)<br>
>>> pointer would help cement that for me. "Oh, I see, AllChains owns the<br>
>>> Chains and ActiveChains has non-owning pointers to those same Chains"<br>
>>> is how I'd read that.<br>
>>><br>
>>> ><br>
>>> > Secondly the loops become more clunky. From:<br>
>>> ><br>
>>> > for (auto *I : AllChains)<br>
>>> > EC.insert(I);<br>
>>> ><br>
>>> > for (auto *I : AllChains) {<br>
>>> > for (auto *J : AllChains) {<br>
>>> > if (I != J && I->rangeOverlapsWith(J))<br>
>>> > EC.unionSets(I, J);<br>
>>> > }<br>
>>> > }<br>
>>> ><br>
>>> ><br>
>>> > to:<br>
>>> ><br>
>>> > for (auto &I : AllChains)<br>
>>> > EC.insert(I.get());<br>
>>><br>
>>> This one's just a necessary thing - I don't think it's overly clunky.<br>
>>> (I could think of some other things to do, but they wouldn't really be<br>
>>> better)<br>
>>><br>
>>> > for (auto &I : AllChains) {<br>
>>> > for (auto &J : AllChains) {<br>
>>> > if (&*I != &*J && I->rangeOverlapsWith(&*J))<br>
>>> > EC.unionSets(&*I, &*J);<br>
>>><br>
>>> This bit I /think/ is just your lack of familiarity, and perhaps a few<br>
>>> other things. std::unique_ptr has operator== so you can just do "I ==<br>
>>> J" as you would with raw pointers.<br>
>>><br>
>>> As for rangeOverlapsWith and unionSets - I've been using unique_ptr as<br>
>>> a great excuse to make more functions take references rather than<br>
>>> pointers. If unionSets and rangeOverlapsWith both took Chain& instead<br>
>>> of Chain*, then the call sites with unique_ptr or raw pointer would be<br>
>>> the same:<br>
>>><br>
>>> rangeOverlapsWith(*J)<br>
>>> unionSets(*I, *J)<br>
>>><br>
>>> And their contracts are clearer - the functions can't take<br>
>>> null/non-values, etc.<br>
>>><br>
>>> > }<br>
>>> > }<br>
>>> ><br>
>>> > (where I've written &* I could have used .get()).<br>
>>> ><br>
>>> > Am I misusing the idiom? What would you recommend?<br>
>>> ><br>
>>> > Cheers,<br>
>>> ><br>
>>> > James<br>
>>> ><br>
>>> ><br>
>>> ><br>
>>> > On 8 August 2014 16:27, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
>>> >><br>
>>> >> On Fri, Aug 8, 2014 at 5:43 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk">james@jamesmolloy.co.uk</a>><br>
>>> >> wrote:<br>
>>> >> > Thanks Tim,<br>
>>> >> ><br>
>>> >> > Using unique_ptr ended up making the code harder to read, so I<br>
>>> >> > simply<br>
>>> >> > added<br>
>>> >> > a deletion loop which I think is cleaner.<br>
>>> >><br>
>>> >> I haven't looked at this patch in detail, but is there a reasonable<br>
>>> >> summary of why unique_ptr made the code harder to read?<br>
>>> >><br>
>>> >> I'm really interested in making/encouraging ownership to be more<br>
>>> >> explicit/clear and would sometimes even encourage it when it hurts<br>
>>> >> (superficial (which might not be the case here)) readability. It's<br>
>>> >> sometimes the case that ownership looks awkward when you make it<br>
>>> >> explicit because it's an awkward ownership scheme and making it<br>
>>> >> implicit just hinds that complexity (in the not good way of hiding -<br>
>>> >> making it a trap/difficult for others to correctly modify the code in<br>
>>> >> the future).<br>
>>> >><br>
>>> >> - David<br>
>>> >><br>
>>> >> ><br>
>>> >> > Committed in r215199, thankyou to all reviewers!<br>
>>> >> ><br>
>>> >> > Cheers,<br>
>>> >> ><br>
>>> >> > James<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > On 8 August 2014 10:42, Tim Northover <<a href="mailto:t.p.northover@gmail.com">t.p.northover@gmail.com</a>><br>
>>> >> > wrote:<br>
>>> >> >><br>
>>> >> >> Just added llvm-commits.<br>
>>> >> >><br>
>>> >> >> ================<br>
>>> >> >> Comment at: lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp:318<br>
>>> >> >> @@ +317,3 @@<br>
>>> >> >> + // "link" register between each inst in the chain.<br>
>>> >> >> + std::map<unsigned, Chain*> ActiveChains;<br>
>>> >> >> + std::set<Chain*> AllChains;<br>
>>> >> >> ----------------<br>
>>> >> >> Tim Northover wrote:<br>
>>> >> >> > Aren't these chains leaked? It *looks* like you could make this<br>
>>> >> >> > (or<br>
>>> >> >> > the<br>
>>> >> >> > set) a unique_ptr<Chain>.<br>
>>> >> >> Also, this should definitely be the set. I hadn't noticed things<br>
>>> >> >> get<br>
>>> >> >> removed from ActiveChains when I wrote this. Should have been<br>
>>> >> >> obvious,<br>
>>> >> >> in<br>
>>> >> >> retrospect.<br>
>>> >> >><br>
>>> >> >> <a href="http://reviews.llvm.org/D4791" target="_blank">http://reviews.llvm.org/D4791</a><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>
>>> >> ><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>
>>> ><br>
>><br>
>><br>
><br>
</div></div></blockquote></div><br></div>