[llvm] r265331 - Revert r263460: [SpillPlacement] Fix a quadratic behavior in spill placement.
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 6 08:57:36 PDT 2016
Hi James,
Wei recommitted r265309 so this one is ready to be reapplied.
How do you want we proceed?
I was thinking you could send me regression test cases I could look into before I recommit this one, but you may have better suggestions :).
Cheers,
-Quentin
> On Apr 5, 2016, at 9:17 AM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>
> Hi James,
>
>> On Apr 5, 2016, at 1:53 AM, James Molloy via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>
>> Hi,
>>
>> For anyone looking to recommit a fixed version of this: the revert *significantly* improved performance on a range of workloads on ARM and AArch64 - affecting ARM more than AArch64 (which is probably expected given the smaller register bank).
>>
>> There were some regressions when reverting this patch, but more improvements than regressions and some were very large indeed.
>
> That is funny, I saw the opposite with X86.
>
>> Some of these will be rubbish purely caused by code motion, but there are so many that I'd need to investigate further.
>>
>> When recommitting, please can we do more performance analysis to work out what this patch is doing? I'm happy to help.
>
> Sure, the problem though is that we trade one heuristic to another and like I said in the original commit, the heuristic finds a local minimum in both cases and thus, there are no good way of biasing it toward the old way or the new way.
> That being said, the new heuristic may expose weaknesses in other part of the register allocator, like what was happening for the asan code, that we may be able to fix.
>
> In the meantime, if you can already file PRs with test cases that regressed with and without the patch, I’d be happy to have a look.
>
> Thanks,
> -Quentin
>
>>
>> Cheers,
>>
>> James
>>
>> On Mon, 4 Apr 2016 at 20:03 Chandler Carruth via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> Author: chandlerc
>> Date: Mon Apr 4 13:57:50 2016
>> New Revision: 265331
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=265331&view=rev <http://llvm.org/viewvc/llvm-project?rev=265331&view=rev>
>> Log:
>> Revert r263460: [SpillPlacement] Fix a quadratic behavior in spill placement.
>>
>> That commit looks wonderful and awesome. Sadly, it greatly exacerbates
>> PR17409 and effectively regresses build time for a lot of (very large)
>> code when compiled with ASan or MSan.
>>
>> We thought this could be fixed forward by landing D15302 which at last
>> fixes that PR, but some issues were discovered and it looks like that
>> got reverted, so reverting this as well temporarily. As soon as the fix
>> for PR17409 lands and sticks, we should re-land this patch as it won't
>> trigger more significant test cases hitting that bug.
>>
>> Many thanks to Quentin and Wei here as they're doing all the awesome
>> hard work!!!
>>
>> 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=265331&r1=265330&r2=265331&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.cpp?rev=265331&r1=265330&r2=265331&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SpillPlacement.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SpillPlacement.cpp Mon Apr 4 13:57:50 2016
>> @@ -173,17 +173,6 @@ 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) {
>> @@ -193,8 +182,6 @@ 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());
>> @@ -212,12 +199,10 @@ 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);
>> @@ -302,6 +287,10 @@ 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);
>> @@ -309,50 +298,76 @@ 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)) {
>> - update(n);
>> + nodes[n].update(nodes, Threshold);
>> // 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() {
>> - // We do not need to push those node in the todolist.
>> - // They are already been proceeded as part of the previous iteration.
>> - RecentPositive.clear();
>> + // 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);
>>
>> - // 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);
>> + 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;
>> +
>> + // 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;
>> }
>> }
>>
>> 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=265331&r1=265330&r2=265331&view=diff <http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SpillPlacement.h?rev=265331&r1=265330&r2=265331&view=diff>
>> ==============================================================================
>> --- llvm/trunk/lib/CodeGen/SpillPlacement.h (original)
>> +++ llvm/trunk/lib/CodeGen/SpillPlacement.h Mon Apr 4 13:57:50 2016
>> @@ -29,7 +29,6 @@
>>
>> #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"
>>
>> @@ -67,9 +66,6 @@ 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.
>>
>> @@ -161,8 +157,6 @@ 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
>
> _______________________________________________
> 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/20160406/d2dd7ef2/attachment.html>
More information about the llvm-commits
mailing list