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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: ELF/CallGraphSort.cpp:10
+///
+/// \file This file implements Call-Chain Clustering from:
+/// Optimizing Function Placement for Large-Scale Data-Center Applications
----------------
Remove `\file` as we don't use this notion in other files.


================
Comment at: ELF/CallGraphSort.cpp:12
+/// Optimizing Function Placement for Large-Scale Data-Center Applications
+/// https://research.fb.com/wp-content/uploads/2017/01/cgo2017-hfsort-final1.pdf
+///
----------------
I don't think a pointer to a paper can be a substitute for a documentation for the code. Can you describe the code here?


================
Comment at: ELF/CallGraphSort.cpp:32
+struct Node {
+  Node() {}
+  Node(const InputSectionBase *IS) {
----------------
`= default`


================
Comment at: ELF/CallGraphSort.cpp:45
+  NodeIndex To;
+  mutable uint64_t Weight;
+  bool operator==(const Edge Other) const {
----------------
Do not make objects const so that we don't have to make this member `mutable`.


================
Comment at: ELF/CallGraphSort.cpp:46-48
+  bool operator==(const Edge Other) const {
+    return From == Other.From && To == Other.To;
+  }
----------------
Please avoid operator overloading as it is not really necessary. You can add a regular function `equals` instead.


================
Comment at: ELF/CallGraphSort.cpp:58-59
+
+typedef std::vector<Node> NodeList;
+typedef std::unordered_set<Edge, EdgeHash> EdgeSet;
+
----------------
We are not using typedefs that often. Please remove and inline them.


================
Comment at: ELF/CallGraphSort.cpp:72
+static void buildCallGraph(NodeList &Nodes, EdgeSet &Edges) {
+  llvm::DenseMap<const InputSectionBase *, NodeIndex> SecToNode;
+
----------------
Remove `llvm::`.


================
Comment at: ELF/CallGraphSort.cpp:74-75
+
+  auto GetOrCreateNode = [&Nodes,
+                          &SecToNode](const InputSectionBase *IS) -> NodeIndex {
+    auto Res = SecToNode.insert(std::make_pair(IS, Nodes.size()));
----------------
`&Nodes, &SecToNode` -> `&`


================
Comment at: ELF/CallGraphSort.cpp:145-151
+  std::sort(Nodes.begin(), Nodes.end(), [](const Node &A, const Node &B) {
+    return (APFloat(APFloat::IEEEdouble(), A.Weight) /
+            APFloat(APFloat::IEEEdouble(), A.Size))
+               .compare(APFloat(APFloat::IEEEdouble(), B.Weight) /
+                        APFloat(APFloat::IEEEdouble(), B.Size)) ==
+           APFloat::cmpLessThan;
+  });
----------------
Do you actually need arbitrary precision floating point numbers?


================
Comment at: ELF/CallGraphSort.cpp:161
+llvm::DenseMap<const InputSectionBase *, int>
+lld::elf::computeCallGraphProfileOrder() {
+  NodeList Nodes;
----------------
Remove `lld::`


================
Comment at: ELF/CallGraphSort.h:19
+namespace elf {
+llvm::DenseMap<const InputSectionBase *, int>
+computeCallGraphProfileOrder();
----------------
Add a declaration for `InputSectionBase` class and remove `#include "InputSection.h"`.


================
Comment at: ELF/Config.h:113
+  llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, uint64_t>
+      CGProfile;
   bool AllowMultipleDefinition;
----------------
Should be `CallgraphOrderingFile` as all Config members are named after their corresponding command line options.


================
Comment at: ELF/Driver.cpp:611-612
+static void readCallGraphProfile(MemoryBufferRef MB) {
+  std::vector<StringRef> Lines = getLines(MB);
+  for (StringRef L : Lines) {
+    SmallVector<StringRef, 3> Fields;
----------------
`for (StringRef L : getLines(MB))`


================
Comment at: ELF/Driver.cpp:616
+    if (Fields.size() != 3)
+      fatal("parse error");
+    uint64_t Count;
----------------
Use `error` and return from this function when you find an error.


================
Comment at: ELF/Writer.cpp:916
+
+  llvm::DenseMap<const InputSectionBase *, int> OrderMap =
+      computeCallGraphProfileOrder();
----------------
ruiu wrote:
> davide wrote:
> > Can you add a comment here?
> Yes, it needs a comment saying that this is for the -callgraph-ordering-file option and that feature is used rarely (so that people who are not interested in the option can skip this part of code when reading lld).
Don't do anything if Config->CGProfile is empty.


================
Comment at: ELF/Writer.cpp:916-925
+  llvm::DenseMap<const InputSectionBase *, int> OrderMap =
+      computeCallGraphProfileOrder();
+
+  for (BaseCommand *Base : Script->Opt.Commands) {
+    auto *OS = dyn_cast<OutputSection>(Base);
+    if (!OS || OS->Name != ".text")
+      continue;
----------------
davide wrote:
> Can you add a comment here?
Yes, it needs a comment saying that this is for the -callgraph-ordering-file option and that feature is used rarely (so that people who are not interested in the option can skip this part of code when reading lld).


https://reviews.llvm.org/D36351