<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40"><head><meta http-equiv=Content-Type content="text/html; charset=utf-8"><meta name=Generator content="Microsoft Word 14 (filtered medium)"><style><!--
/* Font Definitions */
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Wingdings;
panose-1:5 0 0 0 0 0 0 0 0 0;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:Tahoma;
panose-1:2 11 6 4 3 5 4 4 2 4;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0cm;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
p
{mso-style-priority:99;
mso-margin-top-alt:auto;
margin-right:0cm;
mso-margin-bottom-alt:auto;
margin-left:0cm;
font-size:12.0pt;
font-family:"Times New Roman","serif";}
span.EmailStyle18
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri","sans-serif";
mso-fareast-language:EN-US;}
@page WordSection1
{size:612.0pt 792.0pt;
margin:72.0pt 72.0pt 72.0pt 72.0pt;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]--></head><body lang=EN-GB link=blue vlink=purple><div class=WordSection1><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I agree this is the right approach; my patched showed that using TargetSubtargetInfo forces to cross modules boundaries and prevents doing something simple & elegant.<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>Arnaud<o:p></o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal style='margin-left:36.0pt'><b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'>From:</span></b><span lang=EN-US style='font-size:10.0pt;font-family:"Tahoma","sans-serif"'> llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-bounces@cs.uiuc.edu] <b>On Behalf Of </b>Lang Hames<br><b>Sent:</b> 02 October 2014 00:45<br><b>To:</b> David Blaikie<br><b>Cc:</b> Commit Messages and Patches for LLVM<br><b>Subject:</b> Re: [PATCH] Improve the way backends can provide their own PBQP constraints builder<o:p></o:p></span></p><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'>Hi Arnaud, David,<o:p></o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>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.<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>- Lang.<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'>On Thu, Sep 18, 2014 at 12:49 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<o:p></o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'>On Thu, Sep 18, 2014 at 12:35 PM, Arnaud Allard de Grandmaison <<a href="mailto:arnaud.adegm@gmail.com" target="_blank">arnaud.adegm@gmail.com</a>> wrote:<o:p></o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'>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 :) <o:p></o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'>I would leave TargetSubtargetInfo::createPBQPConstraintsBuilder() as it is, consistent with the other methods of TargetSubtargetInfo.<o:p></o:p></p></div></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>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... )<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'> <o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>Cheers,<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>Arnaud<o:p></o:p></p></div></div></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><div><div><p class=MsoNormal style='margin-left:36.0pt'>On Thu, Sep 18, 2014 at 8:49 PM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<o:p></o:p></p></div></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p><div><div><div><p class=MsoNormal style='margin-left:36.0pt'>On Thu, Sep 18, 2014 at 10:12 AM, Arnaud A. de Grandmaison <<a href="mailto:arnaud.degrandmaison@arm.com" target="_blank">arnaud.degrandmaison@arm.com</a>> wrote:<o:p></o:p></p><div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'>Lang, David,<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'> <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'>Following up my PBQP patch for the AArch64, here is a patch improving how targets can provide their own PBQP constraints builder.<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'> <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'>I believe this patch is mostly sane, but could benefit from comments in 2 areas:<o:p></o:p></p><p style='margin-left:36.0pt'>1.<span style='font-size:7.0pt'> </span>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><o:p></o:p></p><p style='margin-left:36.0pt'>2.<span style='font-size:7.0pt'> </span>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 ?<o:p></o:p></p></div></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></div></div><div><p class=MsoNormal style='margin-left:36.0pt'>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>.<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>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. <o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'> <o:p></o:p></p></div><blockquote style='border:none;border-left:solid #CCCCCC 1.0pt;padding:0cm 0cm 0cm 6.0pt;margin-left:4.8pt;margin-right:0cm'><div><div><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'> <o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'>Cheers,<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'>--<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'>Arnaud A. de Grandmaison<o:p></o:p></p><p class=MsoNormal style='mso-margin-top-alt:auto;mso-margin-bottom-alt:auto;margin-left:36.0pt'> <o:p></o:p></p></div></div></blockquote></div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></div><p class=MsoNormal style='mso-margin-top-alt:0cm;margin-right:0cm;margin-bottom:12.0pt;margin-left:36.0pt'>_______________________________________________<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><o:p></o:p></p></blockquote></div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></blockquote></div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></div><p class=MsoNormal style='mso-margin-top-alt:0cm;margin-right:0cm;margin-bottom:12.0pt;margin-left:36.0pt'><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><o:p></o:p></p></div><p class=MsoNormal style='margin-left:36.0pt'><o:p> </o:p></p></div></div></body></html>