[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:17:45 PDT 2016
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 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> 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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160402/df5f8b21/attachment.html>
More information about the llvm-commits
mailing list