<div dir="ltr">Hi Arnaud, David,<div><br></div><div>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.</div><div><br></div><div>- Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Sep 18, 2014 at 12:49 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Thu, Sep 18, 2014 at 12:35 PM, Arnaud Allard de Grandmaison <span dir="ltr"><<a href="mailto:arnaud.adegm@gmail.com" target="_blank">arnaud.adegm@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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 :) <div>I would leave TargetSubtargetInfo::createPBQPConstraintsBuilder() as it is, consistent with the other methods of TargetSubtargetInfo.</div></div></blockquote><div><br></div></span><div>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... )</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><br></div><div>Cheers,</div><div>Arnaud</div></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Thu, Sep 18, 2014 at 8:49 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br></div></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div>On Thu, Sep 18, 2014 at 10:12 AM, Arnaud A. de Grandmaison <span dir="ltr"><<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><p class="MsoNormal">Lang, David,<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">Following up my PBQP patch for the AArch64, here is a patch improving how targets can provide their own PBQP constraints builder.<u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">I believe this patch is mostly sane, but could benefit from comments in 2 areas:<u></u><u></u></p><p><u></u><span>1.<span style="font:7.0pt "Times New Roman""> </span></span><u></u>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 <span style="font-family:Wingdings">J</span><u></u><u></u></p><p><u></u><span>2.<span style="font:7.0pt "Times New Roman""> </span></span><u></u>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 ?</p></div></div></blockquote><div><br></div></div></div><div>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).<br><br>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>.<br></div><div><br></div><div>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. </div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-GB" link="blue" vlink="purple"><div><p><u></u><u></u></p><p class="MsoNormal"><u></u> <u></u></p><p class="MsoNormal">Cheers,<u></u><u></u></p><p class="MsoNormal"><span>--<u></u><u></u></span></p><p class="MsoNormal"><span>Arnaud A. de Grandmaison<u></u><u></u></span></p><p class="MsoNormal"><u></u> <u></u></p></div></div></blockquote></span></div><br></div></div>
<br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>
</blockquote></span></div><br></div></div>
<br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>