[PATCH] [PBQP] Composable constraints.

[Lang Hames]

Make that half-committed in r219421. Missing headers committed in r219425.

- Lang.

On Thu, Oct 9, 2014 at 11:31 AM, Lang Hames <lhames at gmail.com> wrote:

> 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/91b41bbd/attachment.html>