[PATCH] D81429: [COFF] Port CallGraphSort to COFF from ELF

[Hans Wennborg via Phabricator via llvm-commits]

hans accepted this revision.
hans added a comment.
This revision is now accepted and ready to land.

I added some more very minor comments, but overall this looks good to me. Please wait for maskray to sign off too, though.

There doesn't seem to be good documentation for the ELF feature that I could find. I'm not sure where the best way to add it would be (lld's man page?) but if you want, that would be a good thing to improve.



================
Comment at: lld/COFF/CMakeLists.txt:7
   Chunks.cpp
+  CallGraphSort.cpp
   DebugTypes.cpp
----------------
nit: move up one line to make it more alphabetic


================
Comment at: lld/COFF/CallGraphSort.cpp:9
 ///
-/// Implementation of Call-Chain Clustering from: Optimizing Function Placement
-/// for Large-Scale Data-Center Applications
-/// https://research.fb.com/wp-content/uploads/2017/01/cgo2017-hfsort-final1.pdf
-///
-/// The goal of this algorithm is to improve runtime performance of the final
-/// executable by arranging code sections such that page table and i-cache
-/// misses are minimized.
-///
-/// Definitions:
-/// * Cluster
-///   * An ordered list of input sections which are laid out as a unit. At the
-///     beginning of the algorithm each input section has its own cluster and
-///     the weight of the cluster is the sum of the weight of all incoming
-///     edges.
-/// * Call-Chain Clustering (C³) Heuristic
-///   * Defines when and how clusters are combined. Pick the highest weighted
-///     input section then add it to its most likely predecessor if it wouldn't
-///     penalize it too much.
-/// * Density
-///   * The weight of the cluster divided by the size of the cluster. This is a
-///     proxy for the amount of execution time spent per byte of the cluster.
-///
-/// It does so given a call graph profile by the following:
-/// * Build a weighted call graph from the call graph profile
-/// * Sort input sections by weight
-/// * For each input section starting with the highest weight
-///   * Find its most likely predecessor cluster
-///   * Check if the combined cluster would be too large, or would have too low
-///     a density.
-///   * If not, then combine the clusters.
-/// * Sort non-empty clusters by density
+/// This is based on the ELF port, see ELF/ICF.cpp for the details about the
+/// algorithm.
----------------
isn't it lld/ELF/CallGraphSort.cpp?


================
Comment at: lld/COFF/CallGraphSort.cpp:20
 
+#include <cstdio>
 #include <numeric>
----------------
is this actually needed?


================
Comment at: lld/COFF/Driver.cpp:932
+
+  // Build a map from symbol name to section
+  DenseMap<StringRef, Symbol *> map;
----------------
nit: period at the end


================
Comment at: lld/COFF/Driver.cpp:2094
+  if (args.hasArg(OPT_order) && args.hasArg(OPT_call_graph_ordering_file))
+    error("/order and /call-graph-order-file may not be used together");
+
----------------
What happens if the user passes /order but there is also call graphs in the object files?


================
Comment at: lld/COFF/Driver.cpp:2102
 
+  // Handle /call-graph-ordering-file and /call-graph-profile-sort (default on)
+  if (args.hasFlag(OPT_call_graph_profile_sort, OPT_call_graph_profile_sort_no,
----------------
nit: period.


================
Comment at: lld/COFF/Driver.cpp:2111
+
+  // Handle /print-symbol-order
+  if (auto *arg = args.getLastArg(OPT_print_symbol_order))
----------------
nit: period


================
Comment at: lld/COFF/Options.td:246
+    "print-symbol-order",
+    "Print a symbol order specified by /call-graph-ordering-file into the "
+    "specified file">;
----------------
The order could also come from the object files, right? I mean it doesn't have to be specified y /call-graph-ordering-file (but I guess the output is useful for passing to the flag)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81429/new/

https://reviews.llvm.org/D81429