<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Great!<div class=""><br class=""></div><div class="">Out of curiosity, what was the compile time improvement with <a href="http://reviews.llvm.org/D15302" target="_blank" class="">http://reviews.llvm.org/D15302</a>?</div><div class=""><br class=""></div><div class="">Q.<br class=""><div><blockquote type="cite" class=""><div class="">On Apr 2, 2016, at 1:35 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" class="">chandlerc@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class="">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 class=""><div class="gmail_quote"><div dir="ltr" class="">On Sat, Apr 2, 2016 at 1:33 AM Chandler Carruth <<a href="mailto:chandlerc@gmail.com" class="">chandlerc@gmail.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 dir="ltr" class="">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 class=""><div class="gmail_quote"><div dir="ltr" class="">On Sat, Apr 2, 2016 at 1: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:<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="">Hey Chandler,<div class=""><br class=""></div><div class="">After you mentioned the issue on IRC, I figured something like that was happening.</div><div class="">PR17409 was worked around in the sanitizers as fair as I remember and that likely means the workaround does not apply anymore :(.</div><div class=""><br class=""></div><div class="">That being said, I believe there is hope: <a href="http://reviews.llvm.org/D15302" target="_blank" class="">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 class=""><br class=""></div><div class="">Could you check if it fixes your problem?</div><div class=""><br class=""></div><div class="">Thanks,</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 2, 2016, at 1:10 AM, Chandler Carruth <<a href="mailto:chandlerc@gmail.com" target="_blank" class="">chandlerc@gmail.com</a>> wrote:</div><br class=""><div class=""><div dir="ltr" class=""><div class="gmail_quote"><div class="">Hey Quentin,</div><div class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">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 class=""><br class=""></div><div class="">-Chandler</div><div dir="ltr" class=""><br class=""></div><div dir="ltr" class="">On Mon, Mar 14, 2016 at 11:26 AM Quentin Colombet 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: qcolombet<br class="">
Date: Mon Mar 14 13:21:25 2016<br class="">
New Revision: 263460<br class="">
<br class="">
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=263460&view=rev" rel="noreferrer" target="_blank" class="">http://llvm.org/viewvc/llvm-project?rev=263460&view=rev</a><br class="">
Log:<br class="">
[SpillPlacement] Fix a quadratic behavior in spill placement.<br class="">
<br class="">
The bad behavior happens when we have a function with a long linear chain of<br class="">
basic blocks, and have a live range spanning most of this chain, but with very<br class="">
few uses.<br class="">
Let say we have only 2 uses.<br class="">
The Hopfield network is only seeded with two active blocks where the uses are,<br class="">
and each iteration of the outer loop in `RAGreedy::growRegion()` only adds two<br class="">
new nodes to the network due to the completely linear shape of the CFG.<br class="">
Meanwhile, `SpillPlacer->iterate()` visits the whole set of discovered nodes,<br class="">
which adds up to a quadratic algorithm.<br class="">
<br class="">
This is an historical accident effect from r129188.<br class="">
<br class="">
When the Hopfield network is expanding, most of the action is happening on the<br class="">
frontier where new nodes are being added. The internal nodes in the network are<br class="">
not likely to be flip-flopping much, or they will at least settle down very<br class="">
quickly. This means that while `SpillPlacer->iterate()` is recomputing all the<br class="">
nodes in the network, it is probably only the two frontier nodes that are<br class="">
changing their output.<br class="">
<br class="">
Instead of recomputing the whole network on each iteration, we can maintain a<br class="">
SparseSet of nodes that need to be updated:<br class="">
<br class="">
- `SpillPlacement::activate()` adds the node to the todo list.<br class="">
- When a node changes value (i.e., `update()` returns true), its neighbors are<br class="">
added to the todo list.<br class="">
- `SpillPlacement::iterate()` only updates the nodes in the list.<br class="">
<br class="">
The result of Hopfield iterations is not necessarily exact. It should converge<br class="">
to a local minimum, but there is no guarantee that it will find a global<br class="">
minimum. It is possible that updating nodes in a different order will cause us<br class="">
to switch to a different local minimum. In other words, this is not NFC, but<br class="">
although I saw a few runtime improvements and regressions when I benchmarked<br class="">
this change, those were side effects and actually the performance change is in<br class="">
the noise as expected.<br class="">
<br class="">
Huge thanks to Jakob Stoklund Olesen <<a href="mailto:stoklund@2pi.dk" target="_blank" class="">stoklund@2pi.dk</a>> for his feedbacks,<br class="">
guidance and time for the review.<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: <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" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=263460&r1=263459&r2=263460&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/lib/CodeGen/SpillPlacement.cpp (original)<br class="">
+++ llvm/trunk/lib/CodeGen/SpillPlacement.cpp Mon Mar 14 13:21:25 2016<br class="">
@@ -173,6 +173,17 @@ 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="">
@@ -182,6 +193,8 @@ 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="">
@@ -199,10 +212,12 @@ 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="">
@@ -287,10 +302,6 @@ 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="">
@@ -298,76 +309,50 @@ 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="">
- nodes[n].update(nodes, Threshold);<br class="">
+ update(n);<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="">
- // 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="">
- 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="">
+ // 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="">
<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="">
+ // 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="">
}<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: <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" class="">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=263460&r1=263459&r2=263460&view=diff</a><br class="">
==============================================================================<br class="">
--- llvm/trunk/lib/CodeGen/SpillPlacement.h (original)<br class="">
+++ llvm/trunk/lib/CodeGen/SpillPlacement.h Mon Mar 14 13:21:25 2016<br class="">
@@ -29,6 +29,7 @@<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="">
@@ -66,6 +67,9 @@ 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="">
@@ -157,6 +161,8 @@ 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></div>
</div></blockquote></div><br class=""></div></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" rel="noreferrer" target="_blank" class="">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="">
</blockquote></div></blockquote></div>
</div></blockquote></div><br class=""></div></body></html>