<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Sep 18, 2015 at 2:35 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><div><div class="h5">On Fri, Sep 18, 2015 at 2:06 PM, Rui Ueyama via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: ruiu<br>
Date: Fri Sep 18 16:06:34 2015<br>
New Revision: 248038<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=248038&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=248038&view=rev</a><br>
Log:<br>
COFF: Parallelize ICF.<br>
<br>
The LLD's ICF algorithm is highly parallelizable. This patch does that<br>
using parallel_for_each.<br>
<br>
ICF accounted for about one third of total execution time. Previously,<br>
it took 324 ms when self-hosting. Now it takes only 62 ms.<br>
<br>
Of course your mileage may vary. My machine is a beefy 24-core Xeon machine,<br>
so you may not see this much speedup. But this optimization should be<br>
effective even for 2-core machine, since I saw speedup (324 ms -> 189 ms)<br>
when setting parallelism parameter to 2.<br>
<br>
Modified:<br>
lld/trunk/COFF/Chunks.h<br>
lld/trunk/COFF/ICF.cpp<br>
<br>
Modified: lld/trunk/COFF/Chunks.h<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Chunks.h?rev=248038&r1=248037&r2=248038&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/Chunks.h?rev=248038&r1=248037&r2=248038&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/COFF/Chunks.h (original)<br>
+++ lld/trunk/COFF/Chunks.h Fri Sep 18 16:06:34 2015<br>
@@ -17,6 +17,7 @@<br>
#include "llvm/ADT/iterator.h"<br>
#include "llvm/ADT/iterator_range.h"<br>
#include "llvm/Object/COFF.h"<br>
+#include <atomic><br>
#include <map><br>
#include <vector><br>
<br>
@@ -203,7 +204,7 @@ private:<br>
<br>
// Used for ICF (Identical COMDAT Folding)<br>
void replaceWith(SectionChunk *Other);<br>
- uint64_t GroupID;<br>
+ std::atomic<uint64_t> GroupID = { 0 };<br>
<br>
// Chunks are basically unnamed chunks of bytes.<br>
// Symbols are associated for debugging and logging purposs only.<br>
<br>
Modified: lld/trunk/COFF/ICF.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/ICF.cpp?rev=248038&r1=248037&r2=248038&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/lld/trunk/COFF/ICF.cpp?rev=248038&r1=248037&r2=248038&view=diff</a><br>
==============================================================================<br>
--- lld/trunk/COFF/ICF.cpp (original)<br>
+++ lld/trunk/COFF/ICF.cpp Fri Sep 18 16:06:34 2015<br>
@@ -45,6 +45,7 @@<br>
<br>
#include "Chunks.h"<br>
#include "Symbols.h"<br>
+#include "lld/Core/Parallel.h"<br>
#include "llvm/ADT/Hashing.h"<br>
#include "llvm/Support/Debug.h"<br>
#include "llvm/Support/raw_ostream.h"<br>
@@ -56,6 +57,7 @@ using namespace llvm;<br>
namespace lld {<br>
namespace coff {<br>
<br>
+static const size_t NJOBS = 256;<br>
typedef std::vector<SectionChunk *>::iterator ChunkIterator;<br>
typedef bool (*Comparator)(const SectionChunk *, const SectionChunk *);<br>
<br>
@@ -72,7 +74,7 @@ private:<br>
bool partition(ChunkIterator Begin, ChunkIterator End, Comparator Eq);<br>
<br>
const std::vector<Chunk *> &Chunks;<br>
- uint64_t NextID = 0;<br>
+ uint64_t NextID = 1;<br>
};<br>
<br>
// Entry point to ICF.<br>
@@ -179,51 +181,74 @@ bool ICF::forEachGroup(std::vector<Secti<br>
// contents and relocations are all the same.<br>
void ICF::run() {<br>
// Collect only mergeable sections and group by hash value.<br>
- std::vector<SectionChunk *> SChunks;<br>
- for (Chunk *C : Chunks) {<br>
+ std::vector<std::vector<SectionChunk *>> VChunks(NJOBS);<br></blockquote><div><br></div></div></div><div>Could you move this variable down to the desired scope - after the parallel_for_each, just before the for-Chunks loop - then it'll be easier to tell there's no racy parallel use of it.</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ parallel_for_each(Chunks.begin(), Chunks.end(), [&](Chunk *C) {<br>
if (auto *SC = dyn_cast<SectionChunk>(C)) {<br>
bool Writable = SC->getPermissions() & llvm::COFF::IMAGE_SCN_MEM_WRITE;<br>
- if (SC->isCOMDAT() && SC->isLive() && !Writable) {<br>
- SChunks.push_back(SC);<br>
+ if (SC->isCOMDAT() && SC->isLive() && !Writable)<br>
SC->GroupID = getHash(SC) | (uint64_t(1) << 63);<br>
+ }<br>
+ });<br>
+ for (Chunk *C : Chunks) {<br>
+ if (auto *SC = dyn_cast<SectionChunk>(C)) {<br>
+ if (SC->GroupID) {<br>
+ VChunks[SC->GroupID % NJOBS].push_back(SC);<br>
} else {<br>
SC->GroupID = NextID++;<br>
}<br>
}<br>
}<br>
<br>
- // From now on, sections in SChunks are ordered so that sections in<br>
+ // From now on, sections in Chunks are ordered so that sections in<br>
// the same group are consecutive in the vector.<br>
- std::sort(SChunks.begin(), SChunks.end(),<br>
- [](SectionChunk *A, SectionChunk *B) {<br>
- return A->GroupID < B->GroupID;<br>
- });<br>
+ parallel_for_each(VChunks.begin(), VChunks.end(),<br>
+ [&](std::vector<SectionChunk *> &SChunks) {<br>
+ std::sort(SChunks.begin(), SChunks.end(),<br>
+ [](SectionChunk *A, SectionChunk *B) {<br>
+ return A->GroupID < B->GroupID;<br></blockquote><div><br></div></div></div><div>This lambda is in two places - perhaps it should be written in one place (maybe SectionChunk could have op<, then we could use a "deref" variadic functor that just does the dereferencing? I could use more words to describe that if it's interesting/non-obvious).</div><span class=""><div></div></span></div></div></div></blockquote><div><br></div><div>Actually looks like this lambda appears only once.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ });<br>
+ });<br>
<br>
// Split groups until we get a convergence.<br>
int Cnt = 1;<br>
- forEachGroup(SChunks, equalsConstant);<br>
- while (forEachGroup(SChunks, equalsVariable))<br>
+ parallel_for_each(VChunks.begin(), VChunks.end(),<br>
+ [&](std::vector<SectionChunk *> &SChunks) {<br>
+ forEachGroup(SChunks, equalsConstant);<br>
+ });<br>
+<br>
+ for (;;) {<br>
+ bool Redo = false;<br>
+ parallel_for_each(VChunks.begin(), VChunks.end(),<br>
+ [&](std::vector<SectionChunk *> &SChunks) {<br>
+ if (forEachGroup(SChunks, equalsVariable))<br>
+ Redo = true;<br></blockquote><div><br></div></span><div>This looks like a race condition - multiple threads may be racing to write 'true' to Redo.<br><br>(a parallel_find would be a nice abstraction to have, probably)</div><div><div class="h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ });<br>
+ if (!Redo)<br>
+ break;<br>
++Cnt;<br>
+ }<br>
if (Config->Verbose)<br>
llvm::outs() << "\nICF needed " << Cnt << " iterations.\n";<br>
<br>
// Merge sections in the same group.<br>
- for (auto It = SChunks.begin(), End = SChunks.end(); It != End;) {<br>
- SectionChunk *Head = *It;<br>
- auto Bound = std::find_if(It + 1, End, [&](SectionChunk *SC) {<br>
- return Head->GroupID != SC->GroupID;<br>
- });<br>
- if (std::distance(It, Bound) == 1) {</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- It = Bound;<br>
- continue;<br>
- }<br>
- if (Config->Verbose)<br>
- llvm::outs() << "Selected " << Head->getDebugName() << "\n";<br>
- for (++It; It != Bound; ++It) {<br>
- SectionChunk *SC = *It;<br>
+ for (std::vector<SectionChunk *> &SChunks : VChunks) {<br>
+ for (auto It = SChunks.begin(), End = SChunks.end(); It != End;) {<br>
+ SectionChunk *Head = *It;<br>
+ auto Bound = std::find_if(It + 1, End, [&](SectionChunk *SC) {<br>
+ return Head->GroupID != SC->GroupID;<br>
+ });<br>
+ if (std::distance(It, Bound) == 1) {<br></blockquote></div></div><div><br>This could be written as "It + 1 == Bound" (or "std::next(It) == Bound") which may or may not be more legible, up to you. Possibly pull out the It + 1 in the find above, and reuse the value of that expression here.<br><br>(std::distance is particularly useful for generic code, or when you're dealing with a non-random-access-iterator (one that only has increment/decrement, not arbitrary subtraction - it's perhaps more confusing than helpful in the common case like this))<br></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
+ It = Bound;<br>
+ continue;<br>
+ }<br>
if (Config->Verbose)<br>
- llvm::outs() << " Removed " << SC->getDebugName() << "\n";<br>
- SC->replaceWith(Head);<br>
+ llvm::outs() << "Selected " << Head->getDebugName() << "\n";<br>
+ for (++It; It != Bound; ++It) {<br>
+ SectionChunk *SC = *It;<br>
+ if (Config->Verbose)<br>
+ llvm::outs() << " Removed " << SC->getDebugName() << "\n";<br>
+ SC->replaceWith(Head);<br>
+ }<br>
}<br>
}<br>
}<br>
<br>
<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div></div>