[PATCH] [AArch64] FP load balancing pass for Cortex-A57
James Molloy
james at jamesmolloy.co.uk
Mon Aug 11 08:29:54 PDT 2014
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/20140811/dfab59d0/attachment.html>
-------------- next part --------------
Index: lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp
===================================================================
--- lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp (revision 215356)
+++ lib/Target/AArch64/AArch64A57FPLoadBalancing.cpp (working copy)
@@ -141,8 +141,8 @@
bool colorChain(Chain *G, Color C, MachineBasicBlock &MBB);
int scavengeRegister(Chain *G, Color C, MachineBasicBlock &MBB);
void scanInstruction(MachineInstr *MI, unsigned Idx,
- std::map<unsigned, Chain*> &Chains,
- std::set<Chain*> &ChainSet);
+ std::map<unsigned, Chain*> &Active,
+ std::set<std::unique_ptr<Chain>> &AllChains);
void maybeKillChain(MachineOperand &MO, unsigned Idx,
std::map<unsigned, Chain*> &RegChains);
Color getColor(unsigned Register);
@@ -319,7 +319,7 @@
// been killed yet. This is keyed by register - all chains can only have one
// "link" register between each inst in the chain.
std::map<unsigned, Chain*> ActiveChains;
- std::set<Chain*> AllChains;
+ std::set<std::unique_ptr<Chain>> AllChains;
unsigned Idx = 0;
for (auto &MI : MBB)
scanInstruction(&MI, Idx++, ActiveChains, AllChains);
@@ -334,13 +334,15 @@
// range of chains is quite small and they are clustered between loads
// and stores.
EquivalenceClasses<Chain*> EC;
- for (auto *I : AllChains)
- EC.insert(I);
+ 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);
+ 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);
}
}
DEBUG(dbgs() << "Created " << EC.getNumClasses() << " disjoint sets.\n");
@@ -380,9 +382,6 @@
for (auto &I : V)
Changed |= colorChainSet(I, MBB, Parity);
- for (auto *C : AllChains)
- delete C;
-
return Changed;
}
@@ -581,7 +580,7 @@
void AArch64A57FPLoadBalancing::
scanInstruction(MachineInstr *MI, unsigned Idx,
std::map<unsigned, Chain*> &ActiveChains,
- std::set<Chain*> &AllChains) {
+ std::set<std::unique_ptr<Chain>> &AllChains) {
// Inspect "MI", updating ActiveChains and AllChains.
if (isMul(MI)) {
@@ -598,7 +597,7 @@
Chain *G = new Chain(MI, Idx, getColor(DestReg));
ActiveChains[DestReg] = G;
- AllChains.insert(G);
+ AllChains.insert(std::unique_ptr<Chain>(G));
} else if (isMla(MI)) {
@@ -639,7 +638,7 @@
<< TRI->getName(DestReg) << "\n");
Chain *G = new Chain(MI, Idx, getColor(DestReg));
ActiveChains[DestReg] = G;
- AllChains.insert(G);
+ AllChains.insert(std::unique_ptr<Chain>(G));
} else {
More information about the llvm-commits
mailing list