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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

I do not actually understand the algorithm yet, but I'm sending out other review comments so that you can fix them early.

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.



================
Comment at: ELF/CallGraphSort.cpp:165
+
+// Sort sections by the profile data provided by -callgraph-ordering-file
+//
----------------
callgraph-ordering-file -> callgraph-profile-file


================
Comment at: ELF/Config.h:113
+  llvm::DenseMap<std::pair<llvm::StringRef, llvm::StringRef>, uint64_t>
+      CGProfile;
   bool AllowMultipleDefinition;
----------------
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.


================
Comment at: ELF/Driver.cpp:622
+    if (Fields.size() != 3) {
+      error("parse error");
+      return;
----------------
It is better to include more information to an error message:

  error("parse error: " + MB.getBufferIdentifier() + ": " + L);


https://reviews.llvm.org/D36351