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

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese added inline comments.


================
Comment at: ELF/Writer.cpp:894
+// further improve locality.
+template <class ELFT> static void sortByCGProfile() {
+  using NodeIndex = std::ptrdiff_t;
----------------
peter.smith wrote:
> ruiu wrote:
> > Create a new file CallGraph.cpp and move this function there.
> > 
> > Instead of reorder sections in this function, it is probably better to return the OrderMap from this function, and use that map from Writer.cpp, so that this function is side-effect free.
> > 
> > Please split this function into smaller functions.
> It looks like this function could have problems if a Linker Script were used [see bullet below], and I see that the function is currently only called when Linker Scripts are not used. Is there an intention to support this later, or should there be some sort of "not supported" message if someone tries to use a linker script and sortByCGProfile?
> 
> - My understanding is that something like SECTIONS { .text : { *(.text) *(.text.foo) *(.text.bar) } could cause problems if OS->sort([&](InputSectionBase *IS) { return OrderMap.lookup(IS); }); as this function requires exactly 1 InputSectionDescription.
The intent is to support combining with linker scripts at a later time. We currently don't support mixing order files and linker scripts already.


================
Comment at: ELF/Writer.cpp:947
+    // Create the graph.
+    for (const auto &C : Config->CGProfile) {
+      if (C.second == 0)
----------------
peter.smith wrote:
> davide wrote:
> > `auto` when the type is not really obvious isn't a common style in lld, but I'll defer this to Rui.
> By my reading of the code; it looks like sortByCGProfile is always called and doesn't bail out early if CGProfile is empty. May be worth doing to avoid the sort of the .text section. Apologies if I've missed it. 
It's used when the type is obvious or the type is verbose.


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


================
Comment at: ELF/Writer.cpp:996
+      } else
+        ++EI;
+    }
----------------
grimar wrote:
> You missing curly braces:
> ```
> else {
>   ++EI;
> }
> ```
Not needed. llvm style is to omit unneeded braces.


================
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);
+  });
----------------
peter.smith wrote:
> davide wrote:
> > `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.
> Is it ever possible that A.Size is 0? I don't think that this can happen in practice with a correctly generated profile file, but in theory a DefinedRegular in a 0 sized section. 
I've change it to use APFloat so at least it's deterministic (and technically integer math). This may be a case where ScaledNumber makes sense but not sure.


================
Comment at: ELF/Writer.cpp:1013
+  llvm::DenseMap<const InputSectionBase *, std::size_t> OrderMap;
+  ssize_t CurOrder = 0;
+
----------------
davide wrote:
> Any reason for `ssize_t` ? (i.e. why this is not just an unsigned)
You should default to signed unless you need the extra bit of precision or are doing bit twiddling or modulo arithmetic.


================
Comment at: ELF/Writer.cpp:1062
   Script->fabricateDefaultCommands();
+  sortByCGProfile<ELFT>();
   sortBySymbolsOrder<ELFT>();
----------------
ruiu wrote:
> Maybe sortByCallGraph is better, as the meaning of CG is not obvious.
Renamed to computeCallGraphProfileOrder. Long, but only called once so a descriptive name is fine.


================
Comment at: test/ELF/Inputs/cgprofile.txt:1
+5000 c a
+4000 c b
----------------
peter.smith wrote:
> Is it worth a header file in a comment for the profile data with a version number? For example
> // Callgraph Profile Data : Version 1.0 
> This could be used to sanity check that the user had given a sensible input file, and could allow for future changes.
Not sure complicating parsing is worth it.


================
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
+
----------------
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.


================
Comment at: test/ELF/cgprofile.s:5
+
+    .section .text.a,"ax", at progbits
+    .global a
----------------
peter.smith wrote:
> The test currently has profile data for all symbols, is it worth adding some symbols and sections that have not been called to show the effect on the sort order for these sections?
Apparently it would be useful. Found a bug :P. Test added.


https://reviews.llvm.org/D36351