[PATCH] [PBQP] Composable constraints.

[Lang Hames]

Hah. Now I remember why I originally intended to put this on
TargetSubtargetInfo - there's no way to get the sub target from a
TargetRegisterInfo instance.

I'm going to move getCustomPBQPConstraints to TargetSubtargetInfo, and I'll
leave -aarch64-pbqp in place until we figure out a better way to configure
the pass pipeline around the register allocator.

- Lang.


On Thu, Oct 9, 2014 at 7:52 AM, Arnaud A. de Grandmaison <
arnaud.degrandmaison at arm.com> wrote:

>
>
>
>
> *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/b47d8020/attachment.html>