[PATCH] [PBQP] Composable constraints.

[David Blaikie]

On Wed, Oct 8, 2014 at 9:47 PM, Lang Hames <lhames at gmail.com> wrote:

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

Looks good - a few pieces of feedback on new code seemed to be unaddressed
& I'm not sure if they were missed or just differences in opinion. Will
follow up offline, but nothing to fuss about in pre-commit.

- Dave


>
> Thanks all for the review!
>
> Cheers,
> Lang.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141009/78c48cdd/attachment.html>