<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: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;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri","sans-serif";
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
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'><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> 09 October 2014 06:48<br><b>To:</b> David Blaikie<br><b>Cc:</b> Commit Messages and Patches for LLVM<br><b>Subject:</b> Re: [PATCH] [PBQP] Composable constraints.<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'>Thanks for the review Dave.<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'>I've cleaned up cosmetic issues introduced by this patch. Pre-existing blemishes will be tidied up in subsequent patches.<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'>Re functional changes:<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><p class=MsoNormal style='margin-left:36.0pt'>* Unfortunately defaulted and deleted functions are not supported by MSVC2012...<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='margin-left:36.0pt'>Bummer. Removed.<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='margin-left:36.0pt'>- Nodes[NId] = std::move(N);<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>+ Nodes.emplace(Nodes.begin() + NId, std::move(N));<br><br>* You've changed a "modify the existing element" to an "insert an element" - I assume that part was intentional?<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'>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.<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='margin-left:36.0pt'>* 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 <a href="http://llvm.org/docs/CodingStandards.html#provide-a-virtual-method-anchor-for-classes-in-headers" target="_blank">should have an anchor function</a>)<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'>Anchors added.<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><div><div><p class=MsoNormal style='margin-left:36.0pt'>* createPBQPRegisterAllocator - return by unique_ptr?<o:p></o:p></p></div></div></div></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'>Should be. We need to update the RegAllocRegistry stuff along with it, but that's a reasonable change.<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><div><div><div><p class=MsoNormal style='margin-left:36.0pt'>- g.setEdgeCosts(edge, costs);<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>+ G.setEdgeCosts(edge, costs);<br><br>std::move(costs)? You've got similar move usage elsewhere, so I assume that's desired.<o:p></o:p></p></div></div></div></div></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'>Missed optimisation! Thanks for the catch. I've fixed this, and a couple of other instances of the same issue.<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><div><div><div><p class=MsoNormal style='margin-left:36.0pt'>- RegisterRegAlloc::setDefault(createAArch64A57PBQPRegAlloc);<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>+ RegisterRegAlloc::setDefault(createDefaultPBQPRegisterAllocator);<br><br>Isn't that already the default? Do you need to set it again?<o:p></o:p></p></div></div></div></div></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'>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.<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'><span style='color:#1F497D'><o:p> </o:p></span></p><p class=MsoNormal><span style='font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1F497D'>I think AArch64RegisterInfo::getCustomPBQPConstraints() should be modified to check the actual cpu. We only want the A57 contraints … only for the A57 cpu, and default constraints on other cpus.<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'>I agree that aarch64-pbqp should be removed, with the caveat that we want to disable some passes in AArch64TargetMachine.cpp when PBQP is being used with an A57 cpu: this was the use for the UsingPBQP variable. Do you see any good way to do that ? My tentaive patch was also teaching TargetPassConfig to tell if the default register allocator is used or not. I think this can be useful for other targets as well which may need to adapt their pass pipeline to the allocator currently in use.<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></div><div><p class=MsoNormal style='margin-left:36.0pt'>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.<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'>Thanks all for the review!<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'>Cheers,<o:p></o:p></p></div><div><p class=MsoNormal style='margin-left:36.0pt'>Lang.<o:p></o:p></p></div></div></div></div></div></body></html>