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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added inline comments.


================
Comment at: ELF/Writer.cpp:894
+// further improve locality.
+template <class ELFT> static void sortByCGProfile() {
+  using NodeIndex = std::ptrdiff_t;
----------------
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.


================
Comment at: ELF/Writer.cpp:947
+    // Create the graph.
+    for (const auto &C : Config->CGProfile) {
+      if (C.second == 0)
----------------
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. 


================
Comment at: ELF/Writer.cpp:1007
+  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);
----------------
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. 


================
Comment at: test/ELF/Inputs/cgprofile.txt:1
+5000 c a
+4000 c b
----------------
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.


================
Comment at: test/ELF/cgprofile.s:5
+
+    .section .text.a,"ax", at progbits
+    .global a
----------------
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?


Repository:
  rL LLVM

https://reviews.llvm.org/D36351