[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:12
+/// Optimizing Function Placement for Large-Scale Data-Center Applications
+/// https://research.fb.com/wp-content/uploads/2017/01/cgo2017-hfsort-final1.pdf
+///
----------------
Bigcheese wrote:
> 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.
I found that an overview of a file in a few paragraph (or more if it's complicated) as a file comment is very useful to understand code. A paper is a comprehensive source of information, but it is not easy or we do not have enough time to read through it. A brief explanation of an algorithm is pretty helpful.

A comment for each function explains each function, and that is not substitute for a overview comment.


================
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;
+  });
----------------
Bigcheese wrote:
> 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.
Are you saying that this function sorts `Nodes` in a different order if you use `double` instead? It is hard to imagine for me that  some two not-so-huge integers A and B can result in A<B in arbitrary precision calculation but in A==B || A > B if the same computation is done in `double`.


================
Comment at: ELF/Config.h:113
+  llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, uint64_t>
+      CGProfile;
   bool AllowMultipleDefinition;
----------------
Bigcheese wrote:
> 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.
Then can you just drop `File` and name this `CallgraphOrdering`?


Repository:
  rL LLVM

https://reviews.llvm.org/D36351