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

Rui Ueyama via llvm-commits llvm-commits at lists.llvm.org
Mon Nov 27 13:49:41 PST 2017 

I'm so sorry that I didn't respond in a timely manner. I'm reading it again
now.

On Mon, Nov 27, 2017 at 1:41 PM, Davide Italiano <davide at freebsd.org> wrote:

> Rui, any other comments before we push this through the completion line?
> As far as I can tell we all agree on the approach taken, and pcc@
> wanted to extend this work to handle relocations.
> As far as I can tell also Teresa was interested in this work.
> Ideally it will be awesome to have this in 6.0 so downstream consumers
> can pick it up.
>
> On Wed, Nov 1, 2017 at 7:07 PM, Michael Spencer via llvm-commits
> <llvm-commits at lists.llvm.org> wrote:
> > On Thu, Oct 26, 2017 at 7:50 PM, Michael Spencer <bigcheesegs at gmail.com>
> > wrote:
> >>
> >> On Tue, Oct 24, 2017 at 9:30 PM, Rui Ueyama <ruiu at google.com> wrote:
> >>>
> >>> Unfortunately, even with --full-shutdown, the result was the same.
> >>>
> >>
> >> I tested this out and it worked for me with --full-shutdown while
> linking
> >> a trivial object file. One thing I forgot to mention is that you need to
> >> also compile the compiler-rt profile runtime at the same version as
> clang
> >> and llvm. It's probably falling back to an incompatible system version.
> >>
> >> - Michael Spencer
> >
> >
> > I've attached an updated patch based on r317141 that also includes fixes
> for
> > most review comments.
> >
> > - Michael Spencer
> >
> >>
> >>
> >>>
> >>> On Tue, Oct 24, 2017 at 9:25 PM, Rui Ueyama <ruiu at google.com> wrote:
> >>>>
> >>>> On Tue, Oct 24, 2017 at 9:23 PM, Michael Spencer <
> bigcheesegs at gmail.com>
> >>>> wrote:
> >>>>>
> >>>>> On Tue, Oct 24, 2017 at 8:58 PM, Rui Ueyama <ruiu at google.com> wrote:
> >>>>>>
> >>>>>> Sorry, as you pointed out, I made a mistake when I applied the
> >>>>>> patches. Now I succeeded to build it. But the output file of
> llvm-profdata
> >>>>>> seems too small. Is this correct?
> >>>>>>
> >>>>>> This is what I did.
> >>>>>>
> >>>>>> $ CC=`which clang` CXX=`which clang++` /ssd/cmake/bin/cmake -GNinja
> >>>>>> -DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_ENABLE_PROJECTS='clang;
> lld'
> >>>>>> -DCMAKE_C{,XX}_FLAGS=-fprofile-instr-generate ../llvm-project/llvm/
> >>>>>>
> >>>>>> $ ninja clang lld
> >>>>>>
> >>>>>> $ bin/ld.lld <options-to-link-clang>
> >>>>>>
> >>>>>> $ bin/llvm-profdata merge default.profraw -o default.profdata
> >>>>>>
> >>>>>> $ ls -l default.*
> >>>>>> -rw-r----- 1 ruiu eng     560 Oct 24 20:55 default.profdata
> >>>>>> -rw-r----- 1 ruiu eng 2151240 Oct 24 20:55 default.profraw
> >>>>>>
> >>>>>> $ bin/llvm-profdata show default.profraw
> >>>>>> error: default.profraw: Empty raw profile file
> >>>>>
> >>>>>
> >>>>> It could be that lld is using quick exit and not properly flushing
> the
> >>>>> profile data to disk. I'll test out this configuration.
> >>>>
> >>>>
> >>>> Ah, it's likely. I'll try again with `--full-shutdown` option.
> >>>>
> >>>>>
> >>>>> - Michael Spencer
> >>>>>
> >>>>>>
> >>>>>>
> >>>>>> On Mon, Oct 23, 2017 at 7:52 PM, Michael Spencer
> >>>>>> <bigcheesegs at gmail.com> wrote:
> >>>>>>>
> >>>>>>> On Mon, Oct 23, 2017 at 6:53 PM, Rui Ueyama <ruiu at google.com>
> wrote:
> >>>>>>>>
> >>>>>>>> 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(
> >>>>>>>>                ^
> >>>>>>>>
> >>>>>>>
> >>>>>>> Are you sure you applied the patch correctly? I just checked the
> diff
> >>>>>>> and it has all that.
> >>>>>>>
> >>>>>>> diff --git a/ELF/CMakeLists.txt b/ELF/CMakeLists.txt
> >>>>>>> index 205702975..3b3c388d2 100644
> >>>>>>> --- a/ELF/CMakeLists.txt
> >>>>>>> +++ b/ELF/CMakeLists.txt
> >>>>>>> @@ -18,6 +18,7 @@ add_lld_library(lldELF
> >>>>>>>    Arch/SPARCV9.cpp
> >>>>>>>    Arch/X86.cpp
> >>>>>>>    Arch/X86_64.cpp
> >>>>>>> +  CallGraphSort.cpp
> >>>>>>>    Driver.cpp
> >>>>>>>    DriverUtils.cpp
> >>>>>>>    EhFrame.cpp
> >>>>>>>
> >>>>>>> It also has the change that adds CallGraphProfile to Config.h.
> >>>>>>>
> >>>>>>> - Michael Spencer
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 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<DefinedRegular>(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->Section);
> >>>>>>>>>> +    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
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
>
> --
> Davide
>
> "There are no solved problems; there are only problems that are more
> or less solved" -- Henri Poincare
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20171127/890a35c9/attachment.html>

More information about the llvm-commits mailing list