<div dir="ltr">Hah. Now I remember why I originally intended to put this on TargetSubtargetInfo - there's no way to get the sub target from a TargetRegisterInfo instance.<div><br></div><div>I'm going to move getCustomPBQPConstraints to TargetSubtargetInfo, and I'll leave -aarch64-pbqp in place until we figure out a better way to configure the pass pipeline around the register allocator.</div><div><br></div><div>- Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 9, 2014 at 7:52 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"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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""> <a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a> [mailto:<a href="mailto:llvm-commits-bounces@cs.uiuc.edu" target="_blank">llvm-commits-bounces@cs.uiuc.edu</a>] <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.<u></u><u></u></span></p><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p><div><div><div class="h5"><p class="MsoNormal" style="margin-left:36.0pt">Thanks for the review Dave.<u></u><u></u></p><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Re functional changes:<u></u><u></u></p></div></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p><div><div><div class="h5"><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...<u></u><u></u></p></div></blockquote><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Bummer. Removed.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"> <u></u><u></u></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);<u></u><u></u></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?<u></u><u></u></p></div></div></blockquote><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"> <u></u><u></u></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>)<u></u><u></u></p></div></div></blockquote><div><p class="MsoNormal" style="margin-left:36.0pt"> <u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Anchors added.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"> <u></u><u></u></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?<u></u><u></u></p></div></div></div></div></div></blockquote><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></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);<u></u><u></u></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.<u></u><u></u></p></div></div></div></div></div></div></blockquote><div><p class="MsoNormal" style="margin-left:36.0pt"> <u></u><u></u></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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"> <u></u><u></u></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);<u></u><u></u></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?<u></u><u></u></p></div></div></div></div></div></div></blockquote><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></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.<u></u><u></u></p></div></div></div><div><p class="MsoNormal" style="margin-left:36.0pt"><span style="color:#1f497d"><u></u> <u></u></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.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></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.<u></u><u></u></span></p><p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri","sans-serif";color:#1f497d"><u></u> <u></u></span></p></div><span class=""><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.<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Thanks all for the review!<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt"><u></u> <u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Cheers,<u></u><u></u></p></div><div><p class="MsoNormal" style="margin-left:36.0pt">Lang.<u></u><u></u></p></div></span></div></div></div></div></div></blockquote></div><br></div>