[PATCH] [opt] Replace the recursive walk for GC with a worklist algorithm.

[Chandler Carruth]

================
Comment at: COFF/Chunks.h:13
@@ -12,2 +12,3 @@
 
+#include "InputFiles.h"
 #include "lld/Core/LLVM.h"
----------------
ruiu wrote:
> This makes this header and InputFiles.h mutually dependent. Please update InputFiles.h to remove inclusion of this file and add a forward declaration for the class Chunk.
Yea, the prior change fixed that.

================
Comment at: COFF/Chunks.h:141
@@ +140,3 @@
+
+  class symbol_iterator : public llvm::iterator_adaptor_base<
+                              symbol_iterator, const coff_relocation *,
----------------
ruiu wrote:
> symbol_iterator -> SymbolIterator?
This is a common convention in LLVM to name iterators based on the STL naming conventions. I'd really like to avoid deviating from that here.

================
Comment at: COFF/Chunks.h:151-156
@@ +150,8 @@
+
+  public:
+    symbol_iterator() = default;
+
+    SymbolBody *operator*() const {
+      return File->getSymbolBody(I->SymbolTableIndex);
+    }
+  };
----------------
ruiu wrote:
> Move this at beginning of the class definition. Also write operator*() in one line if it fits 80 cols.
Moved, but whether this fits on one line is up to clang-format. =] It formatted it this way, and I don't want to change it.

================
Comment at: COFF/Writer.cpp:118
@@ +117,3 @@
+  // as we push, so sections never appear twice in the list.
+  SmallVector<SectionChunk *, 16> Worklist;
+
----------------
ruiu wrote:
> majnemer wrote:
> > Realistically, won't we always blow out this SmallVector?
> Yeah, this can be much longer than 16. Is SmallVector faster than std::vector for non-small vectors? If not, can we use std::vector instead?
SmallVector is much faster than std::vector in my experience.

Also, we can avoid a bunch of early allocations by starting this fairly high. I've raised the number on the stack to 256 so that we get the early churn out of the way without malloc. I could be convinced it should be closer to 4k if you'd like to wait until we're around a page size before we start hitting the system allocator.

================
Comment at: COFF/Writer.cpp:135
@@ +134,3 @@
+    SectionChunk *SC = Worklist.pop_back_val();
+    assert(SC->isLive() && "We mark as live when pushing onto the worklist!");
+
----------------
ruiu wrote:
> You don't need to call markLive() when you add a new chunk to the Worklist. Instead you can call that function here.
If we fail to call markLive here, then we will add the same SectionChunk to the worklist *many* times. We're essentually using the bool 'Live' state to ensure we only add a chunk to the worklist once.

================
Comment at: COFF/Writer.cpp:146-147
@@ +145,4 @@
+    // Mark associative sections if any.
+    for (Chunk *ChildC : SC->children())
+      if (auto *ChildSC = dyn_cast<SectionChunk>(ChildC))
+        if (!ChildSC->isLive()) {
----------------
ruiu wrote:
> Associative sections are always SectionChunk. You want to change the type to eliminate this dyn_cast.
Done (reflecting your change of the underlying container type).

http://reviews.llvm.org/D10790

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/