<div dir="ltr">Awesome - much more legible. There's a bunch of changes you can/should probably just commit as trivial (moving the PBQP namespace inside the llvm namespace, etc)<br><br>* Unfortunately defaulted and deleted functions are not supported by MSVC2012, so you'll have to write those out explicitly (or rely on the implicit defaults where you can - MSVC2012 also doesn't generate implicit move operations, unfortunately... ). For deleted functions you can make them private and use LLVM_DELETED_FUNCTION or whatever it's called, which is = delete on compilers that support it, and nothing otherwise (so you get better diagnostics where available, and a link error otherwise)<br>* Have you considered the <a href="http://flamingdangerzone.com/cxx11/2012/08/15/rule-of-zero.html">Rule of Zero</a>? (granted, doesn't work so well in MSVC2012 where default move operations are not provided implicitly... )<br><br><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>* I'd recommend using insert, rather than emplace here, as emplace doesn't add any value - insert will do the move operation just fine. You only need to use emplace when you need to perform an explicit conversion (eg: you could pass in a raw pointer to emplace of a vector of unique_ptrs and it would do the right thing). Much like the discussion we've had previously about "unique_ptr u(x());" versus "unique_ptr u = x();" - avoiding code that can invoke explicit conversions tends to make it easier to read because the user doesn't have to be careful that explicit conversions (which are more powerful, possibly more dangerous) are happening.<br><br>* Excess indentation in PBQPRAConstraint.h - we don't generally indent inside namespaces, right? (indeed your previous change was removing a bunch of that indentation in other files) Try running clang-format over the change? (we have a script for that that'll run clang-format over a patch or over a revision range in git, etc)<br><br>* 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">should have an anchor function</a>)<br><br>* SpillCosts could just be a struct (since it inherits publicly and all its members are public)<br>* blank line at the start of SpillCosts::apply seems strange/out of place<br><br>CostMat[I + 1][J + 1] += -Benefit;<br><br>Isn't that the same as "-= Benefit" ?<br><br><div>+ std::unique_ptr<Spiller> VRegSpiller =</div><div>+ std::unique_ptr<Spiller>(createInlineSpiller(*this, MF, VRM));<br><br>A strange piece of code... - I'd omit the std::unique_ptr on the RHS and just use "unique_ptr VRegSpiller(createInlineSpiller(...))" but ideally, createInlineSpiller should be returning unique_ptr and then "auto VRegSpiller = createInlineSpiller(...);"<br><br><div>+ std::unique_ptr<PBQPRAConstraintSet> ConstraintsRoot =</div><div>+ llvm::make_unique<PBQPRAConstraintSet>();<br><br>Up to you, but usually if I'm using make_unique<T> on the RHS, I just use auto on the LHS - the type's already clear from the RHS & doesn't bear repeating.<br><br><div>+ std::string GraphFileName(FullyQualifiedName + "." + RS.str() +</div><div>+ ".pbqpgraph");<br><br>Prefer copy/move initialization (T t = u;) from direct initialization (T t(u);) where possible.<br><br>* createPBQPRegisterAllocator - return by unique_ptr?<br><br><div>- PBQP::Matrix costs(g.getEdgeCosts(edge));</div><div>+ PBQP::Matrix costs(G.getEdgeCosts(edge));<br><br>Copy/move init rather than direct init? (I assume getEdgeCosts returns a PBQP::Matrix, not some other type that's convertible to one)</div><br><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><br><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><br><br></div></div></div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 8, 2014 at 11:32 AM, 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">Hi Dave,<div><br></div><div>Mostly cleaned up patch attached. There are still a few formatting changes, but almost exclusively in places where there are also functional changes.</div><div><br></div><div>Hope this helps.</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>- Lang.</div><div><br></div></font></span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Oct 8, 2014 at 10:08 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">Reformatting makes this patch a bit hard to follow - perhaps you could submit a separate reformat & then rebase the patch for easier review? (or at least when it's committed, so the revision history is a bit easier to follow)<br><br>Yeah, I can't really make heads or tails of which bits of this have semantic changes.</div><div class="gmail_extra"><br><div class="gmail_quote"><span>On Tue, Oct 7, 2014 at 9:05 PM, Lang Hames <span dir="ltr"><<a href="mailto:lhames@gmail.com" target="_blank">lhames@gmail.com</a>></span> wrote:<br></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div><div dir="ltr">Hi All,<div><br></div><div>This patch removes the PBQPBuilder class and its subclasses and replaces them with a composable constraints class: PBQPRAConstraint. This allows constraints that are only required for optimisation (e.g. coalescing, soft pairing) to be mixed and matched. It also makes it easy for targets to supply custom constraints.</div><div><br></div><div>Most of this patch is PBQP-implementation nitty-gritty, but there's one part that I'd like some general feedback on: To enable targets to supply custom constraints I have added a new method to TargetRegisterInfo:</div><div><br></div><div><font face="courier new, monospace">virtual std::unique_ptr<PBQPRAConstraints> getCustomPBQPConstraints() const;</font></div><div><br></div><div>By default this returns a nullptr (indicating no custom constraints are to be used). TargetRegisterInfo seems like a reasonable place to have this, but if I've missed a more appropriate spot it would be good to know. (E.g. If we want to keep TargetRegisterInfo, as much as possible, as an interface for querying the target about the register file then perhaps it would make more sense to put this in TargetSubtargetInfo, but the policy in this area isn't clear to me).</div><div><br></div><div>On the assumption that TargetRegisterInfo is the right place for the aforementioned method, AArch64 has been updated to override this to supply its custom constraints. Arnaud - I'd appreciate it if you could take a quick look and let me know whether you're happy with these AArch64 changes.<br></div><div><br></div><div>This patch should have no impact on allocation quality, and in almost all cases I would expect the resulting allocations to be identical. The only caveat is that PBQP uses FP, and this patch may slightly alter the order of FP operations during initialisation, which could alter some allocations. I would expect that to be very rare, if it happens at all.<br></div><div><br></div><div>Cheers,</div><div>Lang.</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></div></blockquote></div><br></div>