[PATCH] [PBQP] Composable constraints.

[Lang Hames]

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.

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/20141008/59f7357e/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pbqp-composable-constraints-3.patch
Type: application/octet-stream
Size: 65672 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141008/59f7357e/attachment.obj>