[PATCH] D36351: [lld][ELF] Add profile guided section layout

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese added inline comments.


================
Comment at: lld/ELF/CallGraphSort.cpp:194-196
+    } else
+      Edges[EI].Weight = SaturatingAdd(Edges[EI].Weight, Weight);
+
----------------
ruiu wrote:
> This means that an input file could contain more than one line for the same from-to symbol pair. But does that actually happen?
This actually means that multiple symbols could be in the same section.


================
Comment at: lld/ELF/CallGraphSort.cpp:208
+  Edge CE = Edges[CEI];
+  if (CE.From == CE.To) {
+    auto I = WorkLookup.find(CEI);
----------------
ruiu wrote:
> How can the graph have a self edge?
Two symbols which are in the same section.


================
Comment at: lld/ELF/CallGraphSort.cpp:213-215
+    } else
+      assert(std::find(WorkQueue.begin(), WorkQueue.end(), CEI) ==
+             WorkQueue.end());
----------------
ruiu wrote:
> This assertion doesn't make make sense to me -- if `WorkLookup.find(CEI) == WorkLookup.end()`, then `std::find(WorkQueue.begin(), WorkQueue.end(), CEI) == WorkQueue.end()` should be trivially true which can't be broken.
It's not trivially true. WorkLookup and WorkQueue are manually maintained and edges are constantly changed. This is to ensure that they are kept in sync.


================
Comment at: lld/ELF/CallGraphSort.cpp:241
+  // Free memory.
+  std::vector<EdgeIndex>().swap(TE);
+
----------------
ruiu wrote:
> We don't care about micro managing memory consumption in lld. If you need to erase elements not for saving memory but for other reason, please use `erase()`.
> 
Pretty sure we care about N^2 memory usage.


================
Comment at: lld/ELF/CallGraphSort.cpp:248-249
+  // as equal edges will be merged in an order independent manner.
+  std::sort(FE.begin(), FE.end(),
+            [&](EdgeIndex AI, EdgeIndex BI) { return Edges[AI] < Edges[BI]; });
+
----------------
ruiu wrote:
> Can't this just be `std::sort(FE.begin(), FE.end())`?
No.


https://reviews.llvm.org/D36351