<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Thanks for double checking James!<div class=""><br class=""></div><div class="">Q.<br class=""><div><blockquote type="cite" class=""><div class="">On Apr 6, 2016, at 8:59 AM, James Molloy <<a href="mailto:james@jamesmolloy.co.uk" class="">james@jamesmolloy.co.uk</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">Hi Quentin,<div class=""><br class=""></div><div class="">I did a bunch more testing and found my original results to be inconclusive. Although this patch did cause a large swing on a couple of benchmarks, when I ran more benchmarks which are known to have high register pressure it was around 50/50 for improvements.</div><div class=""><br class=""></div><div class="">So let's go ahead - it's just a heuristic after all.</div><div class=""><br class=""></div><div class="">Cheers,</div><div class=""><br class=""></div><div class="">James</div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Wed, 6 Apr 2016 at 16:57 Quentin Colombet <<a href="mailto:qcolombet@apple.com" class="">qcolombet@apple.com</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word" class="">Hi James,<div class=""><br class=""></div><div class="">Wei recommitted r265309 so this one is ready to be reapplied.</div><div class="">How do you want we proceed?</div><div class=""><br class=""></div><div class="">I was thinking you could send me regression test cases I could look into before I recommit this one, but you may have better suggestions :).</div><div class=""><br class=""></div><div class="">Cheers,</div><div class="">-Quentin</div></div><div style="word-wrap:break-word" class=""><div class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Apr 5, 2016, at 9:17 AM, Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class=""><div class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">Hi James,</span><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Apr 5, 2016, at 1:53 AM, James Molloy via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class="">Hi,<div class=""><br class=""></div><div class="">For anyone looking to recommit a fixed version of this: the revert *significantly* improved performance on a range of workloads on ARM and AArch64 - affecting ARM more than AArch64 (which is probably expected given the smaller register bank).</div><div class=""><br class=""></div><div class="">There were some regressions when reverting this patch, but more improvements than regressions and some were very large indeed.</div></div></div></blockquote><div class=""><br class=""></div><div class="">That is funny, I saw the opposite with X86.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="">Some of these will be rubbish purely caused by code motion, but there are so many that I'd need to investigate further.</div><div class=""><br class=""></div><div class="">When recommitting, please can we do more performance analysis to work out what this patch is doing? I'm happy to help.</div></div></div></blockquote><div class=""><br class=""></div><div class="">Sure, the problem though is that we trade one heuristic to another and like I said in the original commit, the heuristic finds a local minimum in both cases and thus, there are no good way of biasing it toward the old way or the new way.</div><div class="">That being said, the new heuristic may expose weaknesses in other part of the register allocator, like what was happening for the asan code, that we may be able to fix.</div><div class=""><br class=""></div><div class="">In the meantime, if you can already file PRs with test cases that regressed with and without the patch, I’d be happy to have a look.</div><div class=""><br class=""></div><div class="">Thanks,</div><div class="">-Quentin</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><br class=""></div><div class="">Cheers,</div><div class=""><br class=""></div><div class="">James</div></div><br class=""><div class="gmail_quote"><div dir="ltr" class="">On Mon, 4 Apr 2016 at 20:03 Chandler Carruth via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a>> wrote:<br class=""></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: chandlerc<br class="">Date: Mon Apr 4 13:57:50 2016<br class="">New Revision: 265331<br class=""><br class="">URL:<span class=""> </span><a href="http://llvm.org/viewvc/llvm-project?rev=265331&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=265331&view=rev</a><br class="">Log:<br class="">Revert r263460: [SpillPlacement] Fix a quadratic behavior in spill placement.<br class=""><br class="">That commit looks wonderful and awesome. Sadly, it greatly exacerbates<br class="">PR17409 and effectively regresses build time for a lot of (very large)<br class="">code when compiled with ASan or MSan.<br class=""><br class="">We thought this could be fixed forward by landing D15302 which at last<br class="">fixes that PR, but some issues were discovered and it looks like that<br class="">got reverted, so reverting this as well temporarily. As soon as the fix<br class="">for PR17409 lands and sticks, we should re-land this patch as it won't<br class="">trigger more significant test cases hitting that bug.<br class=""><br class="">Many thanks to Quentin and Wei here as they're doing all the awesome<br class="">hard work!!!<br class=""><br class="">Modified:<br class=""> llvm/trunk/lib/CodeGen/SpillPlacement.cpp<br class=""> llvm/trunk/lib/CodeGen/SpillPlacement.h<br class=""><br class="">Modified: llvm/trunk/lib/CodeGen/SpillPlacement.cpp<br class="">URL:<span class=""> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=265331&r1=265330&r2=265331&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=265331&r1=265330&r2=265331&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/CodeGen/SpillPlacement.cpp (original)<br class="">+++ llvm/trunk/lib/CodeGen/SpillPlacement.cpp Mon Apr 4 13:57:50 2016<br class="">@@ -173,17 +173,6 @@ struct SpillPlacement::Node {<br class=""> Value = 0;<br class=""> return Before != preferReg();<br class=""> }<br class="">-<br class="">- void getDissentingNeighbors(SparseSet<unsigned> &List,<br class="">- const Node nodes[]) const {<br class="">- for (const auto &Elt : Links) {<br class="">- unsigned n = Elt.second;<br class="">- // Neighbors that already have the same value are not going to<br class="">- // change because of this node changing.<br class="">- if (Value != nodes[n].Value)<br class="">- List.insert(n);<br class="">- }<br class="">- }<br class=""> };<br class=""><br class=""> bool SpillPlacement::runOnMachineFunction(MachineFunction &mf) {<br class="">@@ -193,8 +182,6 @@ bool SpillPlacement::runOnMachineFunctio<br class=""><br class=""> assert(!nodes && "Leaking node array");<br class=""> nodes = new Node[bundles->getNumBundles()];<br class="">- TodoList.clear();<br class="">- TodoList.setUniverse(bundles->getNumBundles());<br class=""><br class=""> // Compute total ingoing and outgoing block frequencies for all bundles.<br class=""> BlockFrequencies.resize(mf.getNumBlockIDs());<br class="">@@ -212,12 +199,10 @@ bool SpillPlacement::runOnMachineFunctio<br class=""> void SpillPlacement::releaseMemory() {<br class=""> delete[] nodes;<br class=""> nodes = nullptr;<br class="">- TodoList.clear();<br class=""> }<br class=""><br class=""> /// activate - mark node n as active if it wasn't already.<br class=""> void SpillPlacement::activate(unsigned n) {<br class="">- TodoList.insert(n);<br class=""> if (ActiveNodes->test(n))<br class=""> return;<br class=""> ActiveNodes->set(n);<br class="">@@ -302,6 +287,10 @@ void SpillPlacement::addLinks(ArrayRef<u<br class=""> continue;<br class=""> activate(ib);<br class=""> activate(ob);<br class="">+ if (nodes[ib].Links.empty() && !nodes[ib].mustSpill())<br class="">+ Linked.push_back(ib);<br class="">+ if (nodes[ob].Links.empty() && !nodes[ob].mustSpill())<br class="">+ Linked.push_back(ob);<br class=""> BlockFrequency Freq = BlockFrequencies[Number];<br class=""> nodes[ib].addLink(ob, Freq);<br class=""> nodes[ob].addLink(ib, Freq);<br class="">@@ -309,50 +298,76 @@ void SpillPlacement::addLinks(ArrayRef<u<br class=""> }<br class=""><br class=""> bool SpillPlacement::scanActiveBundles() {<br class="">+ Linked.clear();<br class=""> RecentPositive.clear();<br class=""> for (int n = ActiveNodes->find_first(); n>=0; n = ActiveNodes->find_next(n)) {<br class="">- update(n);<br class="">+ nodes[n].update(nodes, Threshold);<br class=""> // A node that must spill, or a node without any links is not going to<br class=""> // change its value ever again, so exclude it from iterations.<br class=""> if (nodes[n].mustSpill())<br class=""> continue;<br class="">+ if (!nodes[n].Links.empty())<br class="">+ Linked.push_back(n);<br class=""> if (nodes[n].preferReg())<br class=""> RecentPositive.push_back(n);<br class=""> }<br class=""> return !RecentPositive.empty();<br class=""> }<br class=""><br class="">-bool SpillPlacement::update(unsigned n) {<br class="">- if (!nodes[n].update(nodes, Threshold))<br class="">- return false;<br class="">- nodes[n].getDissentingNeighbors(TodoList, nodes);<br class="">- return true;<br class="">-}<br class="">-<br class=""> /// iterate - Repeatedly update the Hopfield nodes until stability or the<br class=""> /// maximum number of iterations is reached.<br class="">+/// @param Linked - Numbers of linked nodes that need updating.<br class=""> void SpillPlacement::iterate() {<br class="">- // We do not need to push those node in the todolist.<br class="">- // They are already been proceeded as part of the previous iteration.<br class="">- RecentPositive.clear();<br class="">+ // First update the recently positive nodes. They have likely received new<br class="">+ // negative bias that will turn them off.<br class="">+ while (!RecentPositive.empty())<br class="">+ nodes[RecentPositive.pop_back_val()].update(nodes, Threshold);<br class=""><br class="">- // Since the last iteration, the todolist have been augmented by calls<br class="">- // to addConstraints, addLinks, and co.<br class="">- // Update the network energy starting at this new frontier.<br class="">- // The call to ::update will add the nodes that changed into the todolist.<br class="">- unsigned Limit = bundles->getNumBundles() * 10;<br class="">- while(Limit-- > 0 && !TodoList.empty()) {<br class="">- unsigned n = TodoList.pop_back_val();<br class="">- if (!update(n))<br class="">- continue;<br class="">- if (nodes[n].preferReg())<br class="">- RecentPositive.push_back(n);<br class="">+ if (Linked.empty())<br class="">+ return;<br class="">+<br class="">+ // Run up to 10 iterations. The edge bundle numbering is closely related to<br class="">+ // basic block numbering, so there is a strong tendency towards chains of<br class="">+ // linked nodes with sequential numbers. By scanning the linked nodes<br class="">+ // backwards and forwards, we make it very likely that a single node can<br class="">+ // affect the entire network in a single iteration. That means very fast<br class="">+ // convergence, usually in a single iteration.<br class="">+ for (unsigned iteration = 0; iteration != 10; ++iteration) {<br class="">+ // Scan backwards, skipping the last node when iteration is not zero. When<br class="">+ // iteration is not zero, the last node was just updated.<br class="">+ bool Changed = false;<br class="">+ for (SmallVectorImpl<unsigned>::const_reverse_iterator I =<br class="">+ iteration == 0 ? Linked.rbegin() : std::next(Linked.rbegin()),<br class="">+ E = Linked.rend(); I != E; ++I) {<br class="">+ unsigned n = *I;<br class="">+ if (nodes[n].update(nodes, Threshold)) {<br class="">+ Changed = true;<br class="">+ if (nodes[n].preferReg())<br class="">+ RecentPositive.push_back(n);<br class="">+ }<br class="">+ }<br class="">+ if (!Changed || !RecentPositive.empty())<br class="">+ return;<br class="">+<br class="">+ // Scan forwards, skipping the first node which was just updated.<br class="">+ Changed = false;<br class="">+ for (SmallVectorImpl<unsigned>::const_iterator I =<br class="">+ std::next(Linked.begin()), E = Linked.end(); I != E; ++I) {<br class="">+ unsigned n = *I;<br class="">+ if (nodes[n].update(nodes, Threshold)) {<br class="">+ Changed = true;<br class="">+ if (nodes[n].preferReg())<br class="">+ RecentPositive.push_back(n);<br class="">+ }<br class="">+ }<br class="">+ if (!Changed || !RecentPositive.empty())<br class="">+ return;<br class=""> }<br class=""> }<br class=""><br class=""> void SpillPlacement::prepare(BitVector &RegBundles) {<br class="">+ Linked.clear();<br class=""> RecentPositive.clear();<br class="">- TodoList.clear();<br class=""> // Reuse RegBundles as our ActiveNodes vector.<br class=""> ActiveNodes = &RegBundles;<br class=""> ActiveNodes->clear();<br class=""><br class="">Modified: llvm/trunk/lib/CodeGen/SpillPlacement.h<br class="">URL:<span class=""> </span><a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=265331&r1=265330&r2=265331&view=diff" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=265331&r1=265330&r2=265331&view=diff</a><br class="">==============================================================================<br class="">--- llvm/trunk/lib/CodeGen/SpillPlacement.h (original)<br class="">+++ llvm/trunk/lib/CodeGen/SpillPlacement.h Mon Apr 4 13:57:50 2016<br class="">@@ -29,7 +29,6 @@<br class=""><br class=""> #include "llvm/ADT/ArrayRef.h"<br class=""> #include "llvm/ADT/SmallVector.h"<br class="">-#include "llvm/ADT/SparseSet.h"<br class=""> #include "llvm/CodeGen/MachineFunctionPass.h"<br class=""> #include "llvm/Support/BlockFrequency.h"<br class=""><br class="">@@ -67,9 +66,6 @@ class SpillPlacement : public MachineFun<br class=""> /// its inputs falls in the open interval (-Threshold;Threshold).<br class=""> BlockFrequency Threshold;<br class=""><br class="">- /// List of nodes that need to be updated in ::iterate.<br class="">- SparseSet<unsigned> TodoList;<br class="">-<br class=""> public:<br class=""> static char ID; // Pass identification, replacement for typeid.<br class=""><br class="">@@ -161,8 +157,6 @@ private:<br class=""><br class=""> void activate(unsigned);<br class=""> void setThreshold(const BlockFrequency &Entry);<br class="">-<br class="">- bool update(unsigned);<br class=""> };<br class=""><br class=""> } // end namespace llvm<br class=""><br class=""><br class="">_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class=""></blockquote></div>_______________________________________________<br class="">llvm-commits mailing list<br class=""><a href="mailto:llvm-commits@lists.llvm.org" target="_blank" class="">llvm-commits@lists.llvm.org</a><br class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class=""></div></blockquote></div><br class=""></div><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">_______________________________________________</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important" class="">llvm-commits mailing list</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><a href="mailto:llvm-commits@lists.llvm.org" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank" class="">llvm-commits@lists.llvm.org</a><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" class=""><a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" style="font-family:Helvetica;font-size:12px;font-style:normal;font-weight:normal;letter-spacing:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a></div></blockquote></div><br class=""></div></div></blockquote></div>
</div></blockquote></div><br class=""></div></body></html>