<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">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>