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

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese marked 14 inline comments as done.
Bigcheese 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
----------------
ruiu wrote:
> Remove `\file` as we don't use this notion in other files.
https://llvm.org/docs/CodingStandards.html#file-headers

And other parts of lld do use it.


================
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
+///
----------------
ruiu wrote:
> 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?
Code is documented below, if anything in unclear I can add more. Coding standards doc says that the purpose of the file header is to describe the purpose of the file and provide a link to the algorithm.


================
Comment at: ELF/CallGraphSort.cpp:45
+  NodeIndex To;
+  mutable uint64_t Weight;
+  bool operator==(const Edge Other) const {
----------------
ruiu wrote:
> Do not make objects const so that we don't have to make this member `mutable`.
This is needed as `Edge` is used as a unordered_map key which needs to be modified.


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


================
Comment at: ELF/CallGraphSort.cpp:109
+    // Find the largest edge
+    // FIXME: non deterministic order for equal edges.
+    // FIXME: n^2 on Edges.
----------------
davide wrote:
> How hard is to make this a stable sort?
To make this deterministic with the current algorithm a hash map with deterministic iteration order would need to be used (for both edges and `Config->CGProfile`). I didn't focus on it because the algorithm needs to be reworked anyway to not be n^2.


================
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;
+  });
----------------
ruiu wrote:
> Do you actually need arbitrary precision floating point numbers?
We need deterministic results. This is the easiest way to get that in llvm.


================
Comment at: ELF/Config.h:113
+  llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, uint64_t>
+      CGProfile;
   bool AllowMultipleDefinition;
----------------
ruiu wrote:
> Should be `CallgraphOrderingFile` as all Config members are named after their corresponding command line options.
That won't be the only way this member is updated, and it's not a file.


Repository:
  rL LLVM

https://reviews.llvm.org/D36351