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

James Molloy james at jamesmolloy.co.uk
Fri Sep 12 07:45:14 PDT 2014


Hi David,

Committed as r217682.

Cheers!

James

On 26 August 2014 18:25, David Blaikie <dblaikie at gmail.com> wrote:

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


More information about the llvm-commits mailing list