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

[Davide Italiano via Phabricator via llvm-commits]

davide requested changes to this revision.
davide added a comment.
This revision now requires changes to proceed.

Some comments.



================
Comment at: ELF/Writer.cpp:947
+    // Create the graph.
+    for (const auto &C : Config->CGProfile) {
+      if (C.second == 0)
----------------
`auto` when the type is not really obvious isn't a common style in lld, but I'll defer this to Rui.


================
Comment at: ELF/Writer.cpp:990
+    std::vector<Edge> OldEdges;
+    // FIXME: n^2
+    for (auto EI = Edges.begin(), EE = Edges.end(); EI != EE;) {
----------------
This comment is too short to be useful.
We also discussed offline a potential strategy for making this faster, have you tried that?


================
Comment at: ELF/Writer.cpp:1007-1008
+  std::sort(Nodes.begin(), Nodes.end(), [](const Node &A, const Node &B) {
+    return double(A.Weight) / double(A.Size) <
+           double(B.Weight) / double(B.Size);
+  });
----------------
`static_cast<>` maybe? 
FWIW, this code is not great, as all the math dealing with floating point.
It would be awesome if we could use integer math here, instead.


================
Comment at: ELF/Writer.cpp:1013
+  llvm::DenseMap<const InputSectionBase *, std::size_t> OrderMap;
+  ssize_t CurOrder = 0;
+
----------------
Any reason for `ssize_t` ? (i.e. why this is not just an unsigned)


================
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
+
----------------
Probably easier to read if you generate a GNU-compatible output (there's a readobj flag).


Repository:
  rL LLVM

https://reviews.llvm.org/D36351