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