[PATCH] Improve the way backends can provide their own PBQP constraints builder

[Lang Hames]

Hi Arnaud, David,

Taking another look at this, I think the right long term solution is to fix
PBQPBuilder up to be an interface only (i.e. all virtual methods), and move
it into libTarget. Then we can just stick to the clean solution of
returning a std::unique_ptr<PBQPBuilder>. I'm working on a patch for this
now.

- Lang.


On Thu, Sep 18, 2014 at 12:49 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Thu, Sep 18, 2014 at 12:35 PM, Arnaud Allard de Grandmaison <
> arnaud.adegm at gmail.com> wrote:
>
>> I was hoping for something more lightweight; I do not feel the extra
>> complexity brought by the factory is really worth it in this particular
>> case :)
>> I would leave TargetSubtargetInfo::createPBQPConstraintsBuilder() as it
>> is, consistent with the other methods of TargetSubtargetInfo.
>>
>
> Consistent with which other methods? The other methods returning pointers
> return non-owning pointers-to-const data. So they're rather different. (&
> the difference is a bit tricky - now some of the raw pointers are
> non-owning, and one is owning... )
>
>
>>
>> Cheers,
>> Arnaud
>>
>> On Thu, Sep 18, 2014 at 8:49 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Thu, Sep 18, 2014 at 10:12 AM, Arnaud A. de Grandmaison <
>>> arnaud.degrandmaison at arm.com> wrote:
>>>
>>>> Lang, David,
>>>>
>>>>
>>>>
>>>> Following up my PBQP patch for the AArch64, here is a patch improving
>>>> how targets can provide their own PBQP constraints builder.
>>>>
>>>>
>>>>
>>>> I believe this patch is mostly sane, but could benefit from comments
>>>>  in 2 areas:
>>>>
>>>> 1.       How can the backend know which regalloc is being used. In the
>>>> aarch64 case, using pbqp makes some passes useless. I solved that by
>>>> detecting that the default register allocator has been overridden from the
>>>> command line. That’s not perfect, but it is simple & works, and I doubt
>>>> many people will be playing with 3 register allocators at the same time
>>>> J
>>>>
>>>> 2.       Unique_ptr: I avoided coupling Target/TargetSubtargetInfo and
>>>> CodeGen/RegAllocPBQP by using a forward declaration of PBQPBuilder. This
>>>> prevents using a unique_ptr, which wants to see the destructor of the
>>>> object pointed to. This leads to a bit awkward code in
>>>> AArch64Subtarget::createPBQPConstraintsBuilder() where we have to get the
>>>> pointer out of the unique_ptr, to put it back in a unique_ptr in
>>>> createPBQPRegisterAllocator. Dave, would you have any suggestions there ?
>>>>
>>>
>>> I don't think there's a great answer here, unfortunately. If it's just
>>> avoiding a header dependency, then you could make the definition of the
>>> function out of line (but I assume that's insufficient, because you'd still
>>> have a Target dependency on CodeGen).
>>>
>>> If we were pedantic about making the ownership semantics correct here,
>>> you could insert a factory here. Something like "const PBQPBuilderFactory
>>> &getPBQPConstraintsBuilderFactory" (which sounds great, doesn't it?) - the
>>> factory would be owned in the TargetSubtargetInfo implementation for some
>>> particular target (so its definition would only be needed there and by the
>>> callers - but not in the abstract interface). It would just have one
>>> function that returns a unique_ptr<PBQPBuilder>.
>>>
>>> I'm not sure what the 'right' design for APIs are here in
>>> TargetSubtargetInfo - most of the other things here are just chunks of
>>> data, not interactive objects like a BPQPBuilder.
>>>
>>>
>>>>
>>>>
>>>> Cheers,
>>>>
>>>> --
>>>>
>>>> Arnaud A. de Grandmaison
>>>>
>>>>
>>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141001/58d13dc3/attachment.html>