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

[Rui Ueyama via llvm-commits]

It didn't pass cmake because lld/ELF/CMakeLists.txt lacked
"CallGraphSort.cpp", so I added that to the CMakeLists.txt.

It still didn't compile because `Config::CallGraphProfile` doesn't exist,
so I added that boolean variable.

/ssd/llvm-project/lld/ELF/CallGraphSort.cpp:274:29: error: no member named
'CallGraphProfile' in 'lld::elf::Configuration'
  CallGraphSort CGS(Config->CallGraphProfile);
                      ~~~~~~  ^

Now, I'm seeing the following errors. How can I fix them?

/ssd/llvm-project/lld/ELF/CallGraphSort.cpp:274:17: error: no matching
constructor for initialization of '(anonymous namespace)::CallGraphSort'
  CallGraphSort CGS(Config->CallGraphProfile);
                ^   ~~~~~~~~~~~~~~~~~~~~~~~~
/ssd/llvm-project/lld/ELF/CallGraphSort.cpp:43:7: note: candidate
constructor (the implicit copy constructor) not viable: no known conversion
from 'bool' to 'const (anonymous namespace)::CallGraphSort' for 1st argument
class CallGraphSort {
      ^
/ssd/llvm-project/lld/ELF/CallGraphSort.cpp:43:7: note: candidate
constructor (the implicit move constructor) not viable: no known conversion
from 'bool' to '(anonymous namespace)::CallGraphSort' for 1st argument
class CallGraphSort {
      ^
/ssd/llvm-project/lld/ELF/CallGraphSort.cpp:115:16: note: candidate
constructor not viable: no known conversion from 'bool' to
'DenseMap<std::pair<const SymbolBody *, const SymbolBody *>, uint64_t> &'
(aka 'DenseMap<pair<const lld::elf::SymbolBody *, const
lld::elf::SymbolBody *>, unsigned long> &') for 1st argument
CallGraphSort::CallGraphSort(
               ^

On Mon, Oct 23, 2017 at 5:02 PM, Michael Spencer <bigcheesegs at gmail.com>
wrote:

>
>
> - Michael Spencer
>
> On Tue, Oct 3, 2017 at 6:43 PM, Rui Ueyama via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> ruiu added a comment.
>>
>> Could you send me a patch to produce a call graph file so that I can try
>> this patch on my machine?
>>
>
> Sorry for the delay. I've attached the llvm and lld patches that implement
> a full testable version of the feature.
>
> To use:
>
> -1) Have an elf system with working clang instrumentation based profiling
> 0) Compile clang and lld with the supplied patch
> 1) Compile the code with `-fprofile-instr-generate` and link with any
> linker
> 2) Run the program on a representative sample
> 3) `$ llvm-profdata merge default.profraw -o default.profdata`
> 4) Compile the code again with `-fprofile-instr-use=default.profdata
> -ffunction-sections -fuse-ld=lld`
>
> The output of #4 is the program with sections ordered by profile data. You
> can add -Wl,-no-call-graph-profile-sort to disable sorting to measure the
> difference.
>
> `llvm-readobj -elf-cg-profile` will dump the cg profile section.
>
> This also works with LTO.
>
>
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:99-101
>> +  if (To != Other.To)
>> +    return To < Other.To;
>> +  return false;
>> ----------------
>> You can just return `To < Other.To`.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:127
>> +  // Create the graph.
>> +  for (const auto &C : Profile) {
>> +    if (C.second == 0)
>> ----------------
>> This loop is a bit too dense. It cannot be understood without reading
>> each line carefully as I don't understand the whole picture. Please insert
>> a blank line between code blocks. Adding more comment would help.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:128
>> +  for (const auto &C : Profile) {
>> +    if (C.second == 0)
>> +      continue;
>> ----------------
>> Please define local variables for `C.first.first`, `C.first.second` and
>> `C.second` so that they are accessed through meaningful names.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:130-135
>> +    auto FromDR = dyn_cast_or_null<DefinedRegula
>> r>(Symtab->find(C.first.first));
>> +    auto ToDR = dyn_cast_or_null<DefinedRegular>(Symtab->find(C.first.
>> second));
>> +    if (!FromDR || !ToDR)
>> +      continue;
>> +    auto FromSB = dyn_cast_or_null<const InputSectionBase>(FromDR->Sect
>> ion);
>> +    auto ToSB = dyn_cast_or_null<const InputSectionBase>(ToDR->Section);
>> ----------------
>> auto -> auto *
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:147
>> +      Nodes[To].IncidentEdges.push_back(EI);
>> +    } else
>> +      Edges[EI].Weight = SaturatingAdd(Edges[EI].Weight, C.second);
>> ----------------
>> nit: add {}
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:153
>> +
>> +void CallGraphSort::contractEdge(EdgeIndex CEI) {
>> +  // Make a copy of the edge as the original will be marked killed while
>> being
>> ----------------
>> Please add a function comment as to what this function is intended to do.
>> I do not understand this function because I don't get a whole picture.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:158-163
>> +  // Remove the self edge from From.
>> +  FE.erase(std::remove(FE.begin(), FE.end(), CEI));
>> +  std::vector<EdgeIndex> &TE = Nodes[CE.To].IncidentEdges;
>> +  // Update all edges incident with To to reference From instead. Then
>> if they
>> +  // aren't self edges add them to From.
>> +  for (EdgeIndex EI : TE) {
>> ----------------
>> Add blank lines before comments.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:165-166
>> +    Edge &E = Edges[EI];
>> +    // E.From = E.From == CE.To ? CE.From : E.From;
>> +    // E.To = E.To == CE.To ? CE.From : E.To;
>> +    if (E.From == CE.To)
>> ----------------
>> Please remove debug code.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:178-180
>> +  // Free memory.
>> +  std::vector<EdgeIndex>().swap(TE);
>> +
>> ----------------
>> This looks odd. Why do you need to do this? I think you can just leave it
>> alone.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:207-208
>> +
>> +// Group InputSections into clusters using the Call-Chain Clustering
>> heuristic
>> +// then sort the clusters by density.
>> +void CallGraphSort::generateClusters() {
>> ----------------
>> This might be understood for those who read the paper, but I don't think
>> that is enough. Please write more comment as to what are clusters, what is
>> density, and what is the heuristic.
>>
>>
>> ================
>> Comment at: ELF/CallGraphSort.cpp:272-273
>> +DenseMap<const InputSectionBase *, int> elf::computeCallGraphProfileOrder()
>> {
>> +  CallGraphSort CGS(Config->CallGraphProfile);
>> +  return CGS.run();
>> +}
>> ----------------
>> You can do this in one line without defining a local variable.
>>
>>
>> ================
>> Comment at: ELF/Writer.cpp:926-928
>> +    for (BaseCommand *Base : Script->Opt.Commands)
>> +      if (auto *OS = dyn_cast<OutputSection>(Base))
>> +        if (OS->Name == ".text") {
>> ----------------
>> I'd factor this code out as `OutputSection *findOutputSection(StringRef
>> Name)`.
>>
>>
>> https://reviews.llvm.org/D36351
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171023/9c655e1f/attachment.html>