[llvm] r265331 - Revert r263460: [SpillPlacement] Fix a quadratic behavior in spill placement.
James Molloy via llvm-commits
llvm-commits at lists.llvm.org
Wed Apr 6 08:59:56 PDT 2016
Hi Quentin,
I did a bunch more testing and found my original results to be
inconclusive. Although this patch did cause a large swing on a couple of
benchmarks, when I ran more benchmarks which are known to have high
register pressure it was around 50/50 for improvements.
So let's go ahead - it's just a heuristic after all.
Cheers,
James
On Wed, 6 Apr 2016 at 16:57 Quentin Colombet <qcolombet at apple.com> wrote:
> 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> 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> 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
>> 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
>>
>> ==============================================================================
>> --- 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
>>
>> ==============================================================================
>> --- 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
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
> _______________________________________________
> llvm-commits mailing list
> 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
> 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/233dd2af/attachment.html>
More information about the llvm-commits
mailing list