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

[Davide Italiano via Phabricator via llvm-commits]

davide added inline comments.


================
Comment at: ELF/CallGraphSort.cpp:65-66
+  auto Res = Edges.insert(E);
+  if (!Res.second)
+    Res.first->Weight = SaturatingAdd(Res.first->Weight, E.Weight);
+}
----------------
Technically, you don't need to bother running add if you're already at the upper bound, but it's a case unfrequent enough, so I'm not sure it matters.


================
Comment at: ELF/CallGraphSort.cpp:86-88
+    DefinedRegular *FromDR =
+        dyn_cast_or_null<DefinedRegular>(Symtab->find(C.first.first));
+    DefinedRegular *ToDR =
----------------
`auto` because the type is obvious? [also consistent with what you have two lines below]


================
Comment at: ELF/CallGraphSort.cpp:109
+    // Find the largest edge
+    // FIXME: non deterministic order for equal edges.
+    // FIXME: n^2 on Edges.
----------------
How hard is to make this a stable sort?


================
Comment at: ELF/Driver.cpp:608
+//
+// It interprest the first value as an unsigned 64 bit weight, the second as
+// the symbol the call is from, and the third as the symbol the call is to.
----------------
s/interprest/interprets/


================
Comment at: ELF/Driver.cpp:609
+// It interprest the first value as an unsigned 64 bit weight, the second as
+// the symbol the call is from, and the third as the symbol the call is to.
+static void readCallGraphProfile(MemoryBufferRef MB) {
----------------
Maybe add an example :) ?


================
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;
----------------
Can you add a comment here?


================
Comment at: test/ELF/cgprofile.s:2-3
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %s -o %t1
+# RUN: ld.lld %t1 -e a -o %t -callgraph-ordering-file %p/Inputs/cgprofile.txt
+# RUN: llvm-readobj -symbols %t | FileCheck %s
+
----------------
Bigcheese wrote:
> davide wrote:
> > Probably easier to read if you generate a GNU-compatible output (there's a readobj flag).
> The llvm output format was designed specifically for use with FileCheck as it puts everything on a separate line.
OK.


https://reviews.llvm.org/D36351