[PATCH] [AArch64] FP load balancing pass for Cortex-A57

[James Molloy]

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
>> >> >
>> >
>> >
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140826/58f0f041/attachment.html>