<div dir="ltr">Thanks all for the feedback (and Dave, for ongoing discussions on cleanup). Committed in r219421.<br><div><br></div><div>- Lang.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Oct 9, 2014 at 9:40 AM, 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"><div><div class="h5">On Wed, Oct 8, 2014 at 9:47 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@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">Thanks for the review Dave.<div><br></div><div>I've cleaned up cosmetic issues introduced by this patch. Pre-existing blemishes will be tidied up in subsequent patches.</div><div><br></div><div>Re functional changes:</div><div class="gmail_extra"><br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">* Unfortunately defaulted and deleted functions are not supported by MSVC2012...<br></div></blockquote><div><br></div><div>Bummer. Removed.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>- Nodes[NId] = std::move(N);</div><div>+ 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?<br></div></div></blockquote><div><br></div></span><div>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.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>* 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>)<br></div></div></blockquote><div> </div></span><div>Anchors added.</div><span><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><div><div>* createPBQPRegisterAllocator - return by unique_ptr?<br></div></div></div></div></div></blockquote><div><br></div></span><div>Should be. We need to update the RegAllocRegistry stuff along with it, but that's a reasonable change.</div><span><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><div><div><div><div>- g.setEdgeCosts(edge, costs);</div><div>+ G.setEdgeCosts(edge, costs);<br><br>std::move(costs)? You've got similar move usage elsewhere, so I assume that's desired.</div></div></div></div></div></div></blockquote><div> </div></span><div>Missed optimisation! Thanks for the catch. I've fixed this, and a couple of other instances of the same issue.</div><span><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><div><div><div>- RegisterRegAlloc::setDefault(createAArch64A57PBQPRegAlloc);</div><div>+ RegisterRegAlloc::setDefault(createDefaultPBQPRegisterAllocator);<br><br>Isn't that already the default? Do you need to set it again?</div></div></div></div></div></div></blockquote><div><br></div></span><div>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.</div><div><br></div><div>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.</div></div></div></div></blockquote><div><br></div></div></div><div>Looks good - a few pieces of feedback on new code seemed to be unaddressed & I'm not sure if they were missed or just differences in opinion. Will follow up offline, but nothing to fuss about in pre-commit.<br><br>- Dave</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 class="gmail_extra"><div class="gmail_quote"><div><br></div><div>Thanks all for the review!</div><div><br></div><div>Cheers,</div><div>Lang.</div></div></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>