[PATCH] [PBQP] Composable constraints.

[Lang Hames]

Thanks all for the feedback (and Dave, for ongoing discussions on cleanup).
Committed in r219421.

- Lang.

On Thu, Oct 9, 2014 at 9:40 AM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> 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/25181ae0/attachment.html>