[llvm] r263460 - [SpillPlacement] Fix a quadratic behavior in spill placement.
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Sat Apr 2 01:51:10 PDT 2016
Great!
Out of curiosity, what was the compile time improvement with http://reviews.llvm.org/D15302 <http://reviews.llvm.org/D15302>?
Q.
> On Apr 2, 2016, at 1:35 AM, Chandler Carruth <chandlerc at gmail.com> wrote:
>
> 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.
>
> On Sat, Apr 2, 2016 at 1:33 AM Chandler Carruth <chandlerc at gmail.com <mailto:chandlerc at gmail.com>> wrote:
> Cool!!! I didn't realize that was so close to landing! And Tom actually said that this is good to go yesterday I think?
>
> On Sat, Apr 2, 2016 at 1:17 AM Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
> Hey Chandler,
>
> After you mentioned the issue on IRC, I figured something like that was happening.
> PR17409 was worked around in the sanitizers as fair as I remember and that likely means the workaround does not apply anymore :(.
>
> That being said, I believe there is hope: http://reviews.llvm.org/D15302 <http://reviews.llvm.org/D15302> 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.
>
> Could you check if it fixes your problem?
>
> Thanks,
> -Quentin
>
>> On Apr 2, 2016, at 1:10 AM, Chandler Carruth <chandlerc at gmail.com <mailto:chandlerc at gmail.com>> wrote:
>>
>> Hey Quentin,
>>
>> 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....
>>
>> 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.
>>
>> 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.
>>
>> -Chandler
>>
>> On Mon, Mar 14, 2016 at 11:26 AM Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: qcolombet
>> Date: Mon Mar 14 13:21:25 2016
>> New Revision: 263460
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=263460&view=rev <http://llvm.org/viewvc/llvm-project?rev=263460&view=rev>
>> Log:
>> [SpillPlacement] Fix a quadratic behavior in spill placement.
>>
>> The bad behavior happens when we have a function with a long linear chain of
>> basic blocks, and have a live range spanning most of this chain, but with very
>> few uses.
>> Let say we have only 2 uses.
>> The Hopfield network is only seeded with two active blocks where the uses are,
>> and each iteration of the outer loop in `RAGreedy::growRegion()` only adds two
>> new nodes to the network due to the completely linear shape of the CFG.
>> Meanwhile, `SpillPlacer->iterate()` visits the whole set of discovered nodes,
>> which adds up to a quadratic algorithm.
>>
>> This is an historical accident effect from r129188.
>>
>> When the Hopfield network is expanding, most of the action is happening on the
>> frontier where new nodes are being added. The internal nodes in the network are
>> not likely to be flip-flopping much, or they will at least settle down very
>> quickly. This means that while `SpillPlacer->iterate()` is recomputing all the
>> nodes in the network, it is probably only the two frontier nodes that are
>> changing their output.
>>
>> Instead of recomputing the whole network on each iteration, we can maintain a
>> SparseSet of nodes that need to be updated:
>>
>> - `SpillPlacement::activate()` adds the node to the todo list.
>> - When a node changes value (i.e., `update()` returns true), its neighbors are
>> added to the todo list.
>> - `SpillPlacement::iterate()` only updates the nodes in the list.
>>
>> The result of Hopfield iterations is not necessarily exact. It should converge
>> to a local minimum, but there is no guarantee that it will find a global
>> minimum. It is possible that updating nodes in a different order will cause us
>> to switch to a different local minimum. In other words, this is not NFC, but
>> although I saw a few runtime improvements and regressions when I benchmarked
>> this change, those were side effects and actually the performance change is in
>> the noise as expected.
>>
>> Huge thanks to Jakob Stoklund Olesen <stoklund at 2pi.dk <mailto:stoklund at 2pi.dk>> for his feedbacks,
>> guidance and time for the review.
>>
>> Modified:
>> llvm/trunk/lib/CodeGen/SpillPlacement.cpp
>> llvm/trunk/lib/CodeGen/SpillPlacement.h
>>
>> Modified: llvm/trunk/lib/CodeGen/SpillPlacement.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=263460&r1=263459&r2=263460&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=263460&r1=263459&r2=263460&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SpillPlacement.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SpillPlacement.cpp Mon Mar 14 13:21:25 2016
>> @@ -173,6 +173,17 @@ struct SpillPlacement::Node {
>> Value = 0;
>> return Before != preferReg();
>> }
>> +
>> + void getDissentingNeighbors(SparseSet<unsigned> &List,
>> + const Node nodes[]) const {
>> + for (const auto &Elt : Links) {
>> + unsigned n = Elt.second;
>> + // Neighbors that already have the same value are not going to
>> + // change because of this node changing.
>> + if (Value != nodes[n].Value)
>> + List.insert(n);
>> + }
>> + }
>> };
>>
>> bool SpillPlacement::runOnMachineFunction(MachineFunction &mf) {
>> @@ -182,6 +193,8 @@ bool SpillPlacement::runOnMachineFunctio
>>
>> assert(!nodes && "Leaking node array");
>> nodes = new Node[bundles->getNumBundles()];
>> + TodoList.clear();
>> + TodoList.setUniverse(bundles->getNumBundles());
>>
>> // Compute total ingoing and outgoing block frequencies for all bundles.
>> BlockFrequencies.resize(mf.getNumBlockIDs());
>> @@ -199,10 +212,12 @@ bool SpillPlacement::runOnMachineFunctio
>> void SpillPlacement::releaseMemory() {
>> delete[] nodes;
>> nodes = nullptr;
>> + TodoList.clear();
>> }
>>
>> /// activate - mark node n as active if it wasn't already.
>> void SpillPlacement::activate(unsigned n) {
>> + TodoList.insert(n);
>> if (ActiveNodes->test(n))
>> return;
>> ActiveNodes->set(n);
>> @@ -287,10 +302,6 @@ void SpillPlacement::addLinks(ArrayRef<u
>> continue;
>> activate(ib);
>> activate(ob);
>> - if (nodes[ib].Links.empty() && !nodes[ib].mustSpill())
>> - Linked.push_back(ib);
>> - if (nodes[ob].Links.empty() && !nodes[ob].mustSpill())
>> - Linked.push_back(ob);
>> BlockFrequency Freq = BlockFrequencies[Number];
>> nodes[ib].addLink(ob, Freq);
>> nodes[ob].addLink(ib, Freq);
>> @@ -298,76 +309,50 @@ void SpillPlacement::addLinks(ArrayRef<u
>> }
>>
>> bool SpillPlacement::scanActiveBundles() {
>> - Linked.clear();
>> RecentPositive.clear();
>> for (int n = ActiveNodes->find_first(); n>=0; n = ActiveNodes->find_next(n)) {
>> - nodes[n].update(nodes, Threshold);
>> + update(n);
>> // A node that must spill, or a node without any links is not going to
>> // change its value ever again, so exclude it from iterations.
>> if (nodes[n].mustSpill())
>> continue;
>> - if (!nodes[n].Links.empty())
>> - Linked.push_back(n);
>> if (nodes[n].preferReg())
>> RecentPositive.push_back(n);
>> }
>> return !RecentPositive.empty();
>> }
>>
>> +bool SpillPlacement::update(unsigned n) {
>> + if (!nodes[n].update(nodes, Threshold))
>> + return false;
>> + nodes[n].getDissentingNeighbors(TodoList, nodes);
>> + return true;
>> +}
>> +
>> /// iterate - Repeatedly update the Hopfield nodes until stability or the
>> /// maximum number of iterations is reached.
>> -/// @param Linked - Numbers of linked nodes that need updating.
>> void SpillPlacement::iterate() {
>> - // First update the recently positive nodes. They have likely received new
>> - // negative bias that will turn them off.
>> - while (!RecentPositive.empty())
>> - nodes[RecentPositive.pop_back_val()].update(nodes, Threshold);
>> -
>> - if (Linked.empty())
>> - return;
>> -
>> - // Run up to 10 iterations. The edge bundle numbering is closely related to
>> - // basic block numbering, so there is a strong tendency towards chains of
>> - // linked nodes with sequential numbers. By scanning the linked nodes
>> - // backwards and forwards, we make it very likely that a single node can
>> - // affect the entire network in a single iteration. That means very fast
>> - // convergence, usually in a single iteration.
>> - for (unsigned iteration = 0; iteration != 10; ++iteration) {
>> - // Scan backwards, skipping the last node when iteration is not zero. When
>> - // iteration is not zero, the last node was just updated.
>> - bool Changed = false;
>> - for (SmallVectorImpl<unsigned>::const_reverse_iterator I =
>> - iteration == 0 ? Linked.rbegin() : std::next(Linked.rbegin()),
>> - E = Linked.rend(); I != E; ++I) {
>> - unsigned n = *I;
>> - if (nodes[n].update(nodes, Threshold)) {
>> - Changed = true;
>> - if (nodes[n].preferReg())
>> - RecentPositive.push_back(n);
>> - }
>> - }
>> - if (!Changed || !RecentPositive.empty())
>> - return;
>> + // We do not need to push those node in the todolist.
>> + // They are already been proceeded as part of the previous iteration.
>> + RecentPositive.clear();
>>
>> - // Scan forwards, skipping the first node which was just updated.
>> - Changed = false;
>> - for (SmallVectorImpl<unsigned>::const_iterator I =
>> - std::next(Linked.begin()), E = Linked.end(); I != E; ++I) {
>> - unsigned n = *I;
>> - if (nodes[n].update(nodes, Threshold)) {
>> - Changed = true;
>> - if (nodes[n].preferReg())
>> - RecentPositive.push_back(n);
>> - }
>> - }
>> - if (!Changed || !RecentPositive.empty())
>> - return;
>> + // Since the last iteration, the todolist have been augmented by calls
>> + // to addConstraints, addLinks, and co.
>> + // Update the network energy starting at this new frontier.
>> + // The call to ::update will add the nodes that changed into the todolist.
>> + unsigned Limit = bundles->getNumBundles() * 10;
>> + while(Limit-- > 0 && !TodoList.empty()) {
>> + unsigned n = TodoList.pop_back_val();
>> + if (!update(n))
>> + continue;
>> + if (nodes[n].preferReg())
>> + RecentPositive.push_back(n);
>> }
>> }
>>
>> void SpillPlacement::prepare(BitVector &RegBundles) {
>> - Linked.clear();
>> RecentPositive.clear();
>> + TodoList.clear();
>> // Reuse RegBundles as our ActiveNodes vector.
>> ActiveNodes = &RegBundles;
>> ActiveNodes->clear();
>>
>> Modified: llvm/trunk/lib/CodeGen/SpillPlacement.h
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=263460&r1=263459&r2=263460&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=263460&r1=263459&r2=263460&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SpillPlacement.h (original)
>> +++ llvm/trunk/lib/CodeGen/SpillPlacement.h Mon Mar 14 13:21:25 2016
>> @@ -29,6 +29,7 @@
>>
>> #include "llvm/ADT/ArrayRef.h"
>> #include "llvm/ADT/SmallVector.h"
>> +#include "llvm/ADT/SparseSet.h"
>> #include "llvm/CodeGen/MachineFunctionPass.h"
>> #include "llvm/Support/BlockFrequency.h"
>>
>> @@ -66,6 +67,9 @@ class SpillPlacement : public MachineFun
>> /// its inputs falls in the open interval (-Threshold;Threshold).
>> BlockFrequency Threshold;
>>
>> + /// List of nodes that need to be updated in ::iterate.
>> + SparseSet<unsigned> TodoList;
>> +
>> public:
>> static char ID; // Pass identification, replacement for typeid.
>>
>> @@ -157,6 +161,8 @@ private:
>>
>> void activate(unsigned);
>> void setThreshold(const BlockFrequency &Entry);
>> +
>> + bool update(unsigned);
>> };
>>
>> } // end namespace llvm
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160402/01f4e959/attachment.html>
More information about the llvm-commits
mailing list