[PATCH] [PBQP] Composable constraints.

[Arnaud A. de Grandmaison]

From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-bounces at cs.uiuc.edu] On Behalf Of Lang Hames
Sent: 09 October 2014 06:48
To: David Blaikie
Cc: Commit Messages and Patches for LLVM
Subject: Re: [PATCH] [PBQP] Composable constraints.

 

Thanks for the review Dave.

 

I've cleaned up cosmetic issues introduced by this patch. Pre-existing blemishes will be tidied up in subsequent patches.

 

Re functional changes:

 

* Unfortunately defaulted and deleted functions are not supported by MSVC2012...

 

Bummer. Removed.

 

-        Nodes[NId] = std::move(N);

+        Nodes.emplace(Nodes.begin() + NId, std::move(N));

* You've changed a "modify the existing element" to an "insert an element" - I assume that part was intentional?

 

Totally wasn't intentional. That was cruft left over from mucking around with different designs that I'd failed to change back. That code is currently never run, so it hadn't triggered any failures. This code will be exercised in the near future by a Mutable PBQP Graph optimisation that's in the works.

 

* An overriden inline virtual dtor (~PBQPRAConstraintSet) is the default - you can just omit it. (though the LLVM style guide says that any type with a vtable should have an anchor function <http://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers> )

 

Anchors added.

 

* createPBQPRegisterAllocator - return by unique_ptr?

 

Should be. We need to update the RegAllocRegistry stuff along with it, but that's a reasonable change.

 

-      g.setEdgeCosts(edge, costs);

+      G.setEdgeCosts(edge, costs);

std::move(costs)? You've got similar move usage elsewhere, so I assume that's desired.

 

Missed optimisation! Thanks for the catch. I've fixed this, and a couple of other instances of the same issue.

 

-    RegisterRegAlloc::setDefault(createAArch64A57PBQPRegAlloc);

+    RegisterRegAlloc::setDefault(createDefaultPBQPRegisterAllocator);

Isn't that already the default? Do you need to set it again?

 

Greedy is the default allocator. Arnaud is overriding that when the user passes -aarch64-pbqp. I believe we can probably do away with this now (Arnaud - do you see any issue with that?). With this new constraints system -regalloc=pbqp should be sufficient, -aarch64-pbqp is (I think) redundant.

 

I think AArch64RegisterInfo::getCustomPBQPConstraints() should be modified to check the actual cpu. We only want the A57 contraints … only for the A57 cpu, and default constraints on other cpus.

 

I agree that aarch64-pbqp should be removed, with the caveat that we want to disable some passes in AArch64TargetMachine.cpp when PBQP is being used with an A57 cpu: this was the use for the UsingPBQP variable. Do you see any good way to do that ? My tentaive patch was also teaching TargetPassConfig to tell if the default register allocator is used or not. I think this can be useful for other targets as well which may need to adapt their pass pipeline to the allocator currently in use.

 

New patch attached. Arnaud has Ok'd the AArch64 stuff, and Jim has Ok'd the TargetRegisterInfo changes, so as long as this addresses your issues I'll go ahead and commit.

 

Thanks all for the review!

 

Cheers,

Lang.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141009/478efb74/attachment.html>