<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><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><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 class="h5">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 class="h5"><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></div><br></div></div>