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

[Rafael Avila de Espindola via Phabricator via llvm-commits]

espindola accepted this revision.
espindola added a comment.

LGTM with a few last requests.



================
Comment at: ELF/CallGraphSort.cpp:180
+  for (int SI : SortedSecs) {
+    Cluster &C = *SecToCluster[SI];
+
----------------
Since this is looking at a section we haven't merged to a predecessor yet this can be just

Cluster &C = Clusters[SI];


================
Comment at: ELF/CallGraphSort.cpp:185
+
+    for (Edge &E : Clusters[SI].Preds) {
+      if (BestPred == -1 || E.Weight > BestWeight) {
----------------
This can be just C.Preds.


================
Comment at: ELF/CallGraphSort.cpp:192
+
+    if (BestPred == -1)
+      continue;
----------------
You don't think you need this if. If there are no predecessors, BestWeight will be 0 and so the following if will continue since InitialWeight is unsigned.


================
Comment at: ELF/CallGraphSort.cpp:206
+
+    if (isNewDensityBad(*PredC, C))
+      continue;
----------------
All tests pass if I delete this if. Please add a test.


================
Comment at: ELF/CallGraphSort.cpp:212
+    for (int SI : C.Sections)
+      SecToCluster[SI] = PredC;
+
----------------
All tests pass if I delete this loop. Please add a test.


================
Comment at: ELF/CallGraphSort.cpp:224
+
+  // Sort by density. Invalidates all NodeIndexs.
+  std::sort(Clusters.begin(), Clusters.end(),
----------------
NodeIndex is not used elsewhere in the patch. What this (and the previous remove_if) invalidate are the cluster to cluster edges.


https://reviews.llvm.org/D36351