<div dir="ltr">For the test case I have, it went from many minutes to 20-ish seconds. So still slower I think than it was prior to this patch, but not worlds different. And it looks like there is now some stuff at the top of the profile that might be easily improved.<div><br></div><div>My suspicion is that the spill placement code's overly aggressive walking of the list was hiding a lot of work for the rest of the register allocator here. =/ Which is fine, once that patch lands and the rest of the register allocator can cope. =]</div></div><br><div class="gmail_quote"><div dir="ltr">On Sat, Apr 2, 2016 at 1:51 AM Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br></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">Great!<div><br></div><div>Out of curiosity, what was the compile time improvement with <a href="http://reviews.llvm.org/D15302" target="_blank">http://reviews.llvm.org/D15302</a>?</div><div><br></div><div></div></div><div style="word-wrap:break-word"><div>Q.</div></div><div style="word-wrap:break-word"><div><br><div><blockquote type="cite"><div>On Apr 2, 2016, at 1:35 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:</div><br><div><div dir="ltr">I've just tried out that patch, and yes, it fixes my test case!!! Awesome, I'll follow up on that patch to try to make sure it lands promptly.</div><br><div class="gmail_quote"><div dir="ltr">On Sat, Apr 2, 2016 at 1:33 AM Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Cool!!! I didn't realize that was so close to landing! And Tom actually said that this is good to go yesterday I think?</div><br><div class="gmail_quote"><div dir="ltr">On Sat, Apr 2, 2016 at 1:17 AM Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></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">Hey Chandler,<div><br></div><div>After you mentioned the issue on IRC, I figured something like that was happening.</div><div>PR17409 was worked around in the sanitizers as fair as I remember and that likely means the workaround does not apply anymore :(.</div><div><br></div><div>That being said, I believe there is hope: <a href="http://reviews.llvm.org/D15302" target="_blank">http://reviews.llvm.org/D15302</a> should fix that issue once and for all and is ready to go from my point of view. It misses Tom's approval but I think it is a matter of pinging him.</div><div><br></div><div>Could you check if it fixes your problem?</div><div><br></div><div>Thanks,</div><div>-Quentin</div></div><div style="word-wrap:break-word"><div><br><div><blockquote type="cite"><div>On Apr 2, 2016, at 1:10 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank">chandlerc@gmail.com</a>> wrote:</div><br><div><div dir="ltr"><div class="gmail_quote"><div>Hey Quentin,</div><div><br></div><div>This patch seems really, really awesome. However, for reasons that completely baffle me, it is introducing really bad new compile time problems for us. But not in any way I expected....</div><div><br></div><div>What seems to happen is that with this change in place, it changes things in a way I don't understand that makes PR17409 suddenly *much* more common. Now that PR hits for many, many more cases with the sanitizers.</div><div><br></div><div>I'm still digging into this to try to understand why, but thought I'd let you know in case others are running into this as well.</div><div><br></div><div>-Chandler</div><div dir="ltr"><br></div><div dir="ltr">On Mon, Mar 14, 2016 at 11:26 AM Quentin Colombet via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: qcolombet<br>
Date: Mon Mar 14 13:21:25 2016<br>
New Revision: 263460<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263460&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=263460&view=rev</a><br>
Log:<br>
[SpillPlacement] Fix a quadratic behavior in spill placement.<br>
<br>
The bad behavior happens when we have a function with a long linear chain of<br>
basic blocks, and have a live range spanning most of this chain, but with very<br>
few uses.<br>
Let say we have only 2 uses.<br>
The Hopfield network is only seeded with two active blocks where the uses are,<br>
and each iteration of the outer loop in `RAGreedy::growRegion()` only adds two<br>
new nodes to the network due to the completely linear shape of the CFG.<br>
Meanwhile, `SpillPlacer->iterate()` visits the whole set of discovered nodes,<br>
which adds up to a quadratic algorithm.<br>
<br>
This is an historical accident effect from r129188.<br>
<br>
When the Hopfield network is expanding, most of the action is happening on the<br>
frontier where new nodes are being added. The internal nodes in the network are<br>
not likely to be flip-flopping much, or they will at least settle down very<br>
quickly. This means that while `SpillPlacer->iterate()` is recomputing all the<br>
nodes in the network, it is probably only the two frontier nodes that are<br>
changing their output.<br>
<br>
Instead of recomputing the whole network on each iteration, we can maintain a<br>
SparseSet of nodes that need to be updated:<br>
<br>
- `SpillPlacement::activate()` adds the node to the todo list.<br>
- When a node changes value (i.e., `update()` returns true), its neighbors are<br>
added to the todo list.<br>
- `SpillPlacement::iterate()` only updates the nodes in the list.<br>
<br>
The result of Hopfield iterations is not necessarily exact. It should converge<br>
to a local minimum, but there is no guarantee that it will find a global<br>
minimum. It is possible that updating nodes in a different order will cause us<br>
to switch to a different local minimum. In other words, this is not NFC, but<br>
although I saw a few runtime improvements and regressions when I benchmarked<br>
this change, those were side effects and actually the performance change is in<br>
the noise as expected.<br>
<br>
Huge thanks to Jakob Stoklund Olesen <<a href="mailto:stoklund@2pi.dk" target="_blank">stoklund@2pi.dk</a>> for his feedbacks,<br>
guidance and time for the review.<br>
<br>
Modified:<br>
llvm/trunk/lib/CodeGen/SpillPlacement.cpp<br>
llvm/trunk/lib/CodeGen/SpillPlacement.h<br>
<br>
Modified: llvm/trunk/lib/CodeGen/SpillPlacement.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=263460&r1=263459&r2=263460&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=263460&r1=263459&r2=263460&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/CodeGen/SpillPlacement.cpp (original)<br>
+++ llvm/trunk/lib/CodeGen/SpillPlacement.cpp Mon Mar 14 13:21:25 2016<br>
@@ -173,6 +173,17 @@ struct SpillPlacement::Node {<br>
Value = 0;<br>
return Before != preferReg();<br>
}<br>
+<br>
+ void getDissentingNeighbors(SparseSet<unsigned> &List,<br>
+ const Node nodes[]) const {<br>
+ for (const auto &Elt : Links) {<br>
+ unsigned n = Elt.second;<br>
+ // Neighbors that already have the same value are not going to<br>
+ // change because of this node changing.<br>
+ if (Value != nodes[n].Value)<br>
+ List.insert(n);<br>
+ }<br>
+ }<br>
};<br>
<br>
bool SpillPlacement::runOnMachineFunction(MachineFunction &mf) {<br>
@@ -182,6 +193,8 @@ bool SpillPlacement::runOnMachineFunctio<br>
<br>
assert(!nodes && "Leaking node array");<br>
nodes = new Node[bundles->getNumBundles()];<br>
+ TodoList.clear();<br>
+ TodoList.setUniverse(bundles->getNumBundles());<br>
<br>
// Compute total ingoing and outgoing block frequencies for all bundles.<br>
BlockFrequencies.resize(mf.getNumBlockIDs());<br>
@@ -199,10 +212,12 @@ bool SpillPlacement::runOnMachineFunctio<br>
void SpillPlacement::releaseMemory() {<br>
delete[] nodes;<br>
nodes = nullptr;<br>
+ TodoList.clear();<br>
}<br>
<br>
/// activate - mark node n as active if it wasn't already.<br>
void SpillPlacement::activate(unsigned n) {<br>
+ TodoList.insert(n);<br>
if (ActiveNodes->test(n))<br>
return;<br>
ActiveNodes->set(n);<br>
@@ -287,10 +302,6 @@ void SpillPlacement::addLinks(ArrayRef<u<br>
continue;<br>
activate(ib);<br>
activate(ob);<br>
- if (nodes[ib].Links.empty() && !nodes[ib].mustSpill())<br>
- Linked.push_back(ib);<br>
- if (nodes[ob].Links.empty() && !nodes[ob].mustSpill())<br>
- Linked.push_back(ob);<br>
BlockFrequency Freq = BlockFrequencies[Number];<br>
nodes[ib].addLink(ob, Freq);<br>
nodes[ob].addLink(ib, Freq);<br>
@@ -298,76 +309,50 @@ void SpillPlacement::addLinks(ArrayRef<u<br>
}<br>
<br>
bool SpillPlacement::scanActiveBundles() {<br>
- Linked.clear();<br>
RecentPositive.clear();<br>
for (int n = ActiveNodes->find_first(); n>=0; n = ActiveNodes->find_next(n)) {<br>
- nodes[n].update(nodes, Threshold);<br>
+ update(n);<br>
// A node that must spill, or a node without any links is not going to<br>
// change its value ever again, so exclude it from iterations.<br>
if (nodes[n].mustSpill())<br>
continue;<br>
- if (!nodes[n].Links.empty())<br>
- Linked.push_back(n);<br>
if (nodes[n].preferReg())<br>
RecentPositive.push_back(n);<br>
}<br>
return !RecentPositive.empty();<br>
}<br>
<br>
+bool SpillPlacement::update(unsigned n) {<br>
+ if (!nodes[n].update(nodes, Threshold))<br>
+ return false;<br>
+ nodes[n].getDissentingNeighbors(TodoList, nodes);<br>
+ return true;<br>
+}<br>
+<br>
/// iterate - Repeatedly update the Hopfield nodes until stability or the<br>
/// maximum number of iterations is reached.<br>
-/// @param Linked - Numbers of linked nodes that need updating.<br>
void SpillPlacement::iterate() {<br>
- // First update the recently positive nodes. They have likely received new<br>
- // negative bias that will turn them off.<br>
- while (!RecentPositive.empty())<br>
- nodes[RecentPositive.pop_back_val()].update(nodes, Threshold);<br>
-<br>
- if (Linked.empty())<br>
- return;<br>
-<br>
- // Run up to 10 iterations. The edge bundle numbering is closely related to<br>
- // basic block numbering, so there is a strong tendency towards chains of<br>
- // linked nodes with sequential numbers. By scanning the linked nodes<br>
- // backwards and forwards, we make it very likely that a single node can<br>
- // affect the entire network in a single iteration. That means very fast<br>
- // convergence, usually in a single iteration.<br>
- for (unsigned iteration = 0; iteration != 10; ++iteration) {<br>
- // Scan backwards, skipping the last node when iteration is not zero. When<br>
- // iteration is not zero, the last node was just updated.<br>
- bool Changed = false;<br>
- for (SmallVectorImpl<unsigned>::const_reverse_iterator I =<br>
- iteration == 0 ? Linked.rbegin() : std::next(Linked.rbegin()),<br>
- E = Linked.rend(); I != E; ++I) {<br>
- unsigned n = *I;<br>
- if (nodes[n].update(nodes, Threshold)) {<br>
- Changed = true;<br>
- if (nodes[n].preferReg())<br>
- RecentPositive.push_back(n);<br>
- }<br>
- }<br>
- if (!Changed || !RecentPositive.empty())<br>
- return;<br>
+ // We do not need to push those node in the todolist.<br>
+ // They are already been proceeded as part of the previous iteration.<br>
+ RecentPositive.clear();<br>
<br>
- // Scan forwards, skipping the first node which was just updated.<br>
- Changed = false;<br>
- for (SmallVectorImpl<unsigned>::const_iterator I =<br>
- std::next(Linked.begin()), E = Linked.end(); I != E; ++I) {<br>
- unsigned n = *I;<br>
- if (nodes[n].update(nodes, Threshold)) {<br>
- Changed = true;<br>
- if (nodes[n].preferReg())<br>
- RecentPositive.push_back(n);<br>
- }<br>
- }<br>
- if (!Changed || !RecentPositive.empty())<br>
- return;<br>
+ // Since the last iteration, the todolist have been augmented by calls<br>
+ // to addConstraints, addLinks, and co.<br>
+ // Update the network energy starting at this new frontier.<br>
+ // The call to ::update will add the nodes that changed into the todolist.<br>
+ unsigned Limit = bundles->getNumBundles() * 10;<br>
+ while(Limit-- > 0 && !TodoList.empty()) {<br>
+ unsigned n = TodoList.pop_back_val();<br>
+ if (!update(n))<br>
+ continue;<br>
+ if (nodes[n].preferReg())<br>
+ RecentPositive.push_back(n);<br>
}<br>
}<br>
<br>
void SpillPlacement::prepare(BitVector &RegBundles) {<br>
- Linked.clear();<br>
RecentPositive.clear();<br>
+ TodoList.clear();<br>
// Reuse RegBundles as our ActiveNodes vector.<br>
ActiveNodes = &RegBundles;<br>
ActiveNodes->clear();<br>
<br>
Modified: llvm/trunk/lib/CodeGen/SpillPlacement.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=263460&r1=263459&r2=263460&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=263460&r1=263459&r2=263460&view=diff</a><br>
==============================================================================<br>
--- llvm/trunk/lib/CodeGen/SpillPlacement.h (original)<br>
+++ llvm/trunk/lib/CodeGen/SpillPlacement.h Mon Mar 14 13:21:25 2016<br>
@@ -29,6 +29,7 @@<br>
<br>
#include "llvm/ADT/ArrayRef.h"<br>
#include "llvm/ADT/SmallVector.h"<br>
+#include "llvm/ADT/SparseSet.h"<br>
#include "llvm/CodeGen/MachineFunctionPass.h"<br>
#include "llvm/Support/BlockFrequency.h"<br>
<br>
@@ -66,6 +67,9 @@ class SpillPlacement : public MachineFun<br>
/// its inputs falls in the open interval (-Threshold;Threshold).<br>
BlockFrequency Threshold;<br>
<br>
+ /// List of nodes that need to be updated in ::iterate.<br>
+ SparseSet<unsigned> TodoList;<br>
+<br>
public:<br>
static char ID; // Pass identification, replacement for typeid.<br>
<br>
@@ -157,6 +161,8 @@ private:<br>
<br>
void activate(unsigned);<br>
void setThreshold(const BlockFrequency &Entry);<br>
+<br>
+ bool update(unsigned);<br>
};<br>
<br>
} // end namespace llvm<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div>
</div></blockquote></div><br></div></div>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></blockquote></div>
</div></blockquote></div><br></div></div></blockquote></div>