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

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese marked 2 inline comments as done.
Bigcheese added a comment.

> Could you give me a few benchmark numbers and a instruction to reproduce the results, so that we are sure that the feature actually produces better results.

I can probably share some more detailed PS4 numbers than I had in the RFC, but getting publicly reproducible numbers is going to take a while.



================
Comment at: ELF/Config.h:113
+  llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, uint64_t>
+      CGProfile;
   bool AllowMultipleDefinition;
----------------
ruiu wrote:
> Bigcheese wrote:
> > ruiu wrote:
> > > 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`?
> > It's also not an ordering. Which means the name of the option is wrong. Should be -callgraph-profile-file and `CallGraphProfile`.
> Please rename this CallgraphProfile as we generally have a member FoobarBaz in Config for option -foobar-baz.
There are only 2 that follow that, there are 3 that don't, and style guide says to use camel case and capitalize the same word consistently. Everywhere else in llvm it's CallGraph (1 match for Callgraph in a string, 1300 matches for CallGraph).


https://reviews.llvm.org/D36351