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

[James Molloy]

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. 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?).

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());

  for (auto &I : AllChains) {
    for (auto &J : AllChains) {
      if (&*I != &*J && I->rangeOverlapsWith(&*J))
        EC.unionSets(&*I, &*J);
    }
  }

(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/20140808/7f3bdfe8/attachment.html>