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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/CallGraphSort.cpp:208
+  Edge CE = Edges[CEI];
+  if (CE.From == CE.To) {
+    auto I = WorkLookup.find(CEI);
----------------
How can the graph have a self edge?


================
Comment at: lld/ELF/CallGraphSort.cpp:213-215
+    } else
+      assert(std::find(WorkQueue.begin(), WorkQueue.end(), CEI) ==
+             WorkQueue.end());
----------------
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.


================
Comment at: lld/ELF/CallGraphSort.cpp:241
+  // Free memory.
+  std::vector<EdgeIndex>().swap(TE);
+
----------------
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()`.



================
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]; });
+
----------------
Can't this just be `std::sort(FE.begin(), FE.end())`?


================
Comment at: lld/ELF/CallGraphSort.cpp:257
+    if (Edges[*Result] == Edges[*First]) {
+      // Remove first and result.
+      auto F = WorkLookup.find(*First);
----------------
It seems you are not actually removing Result.


================
Comment at: lld/ELF/CallGraphSort.cpp:263-264
+      } else
+        assert(std::find(WorkQueue.begin(), WorkQueue.end(), *First) ==
+               WorkQueue.end());
+      auto R = WorkLookup.find(*Result);
----------------
Remove.


================
Comment at: lld/ELF/CallGraphSort.cpp:267-268
+      if (R == WorkLookup.end()) {
+        assert(std::find(WorkQueue.begin(), WorkQueue.end(), *Result) ==
+               WorkQueue.end());
+        R = WorkLookup.insert(std::make_pair(*Result, WorkQueue.end())).first;
----------------
Remove


================
Comment at: lld/ELF/CallGraphSort.cpp:279
+      R->second = WorkQueue.insert(*Result);
+    } else if (++Result != First)
+      *Result = *First;
----------------
nit: please add {}


================
Comment at: lld/ELF/CallGraphSort.cpp:292
+
+  // Collapse the graph.
+  while (!WorkQueue.empty()) {
----------------
Please expand this comment. In particular, please describe what WorkLookup and WorkQueue are expected to have. Looks like I can't understand the code without it.


https://reviews.llvm.org/D36351