[PATCH] D36351: [lld][ELF] Add profile guided section layout
Michael Spencer via llvm-commits
llvm-commits at lists.llvm.org
Mon Oct 23 19:52:48 PDT 2017
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<DefinedRegula
>>> r>(Symtab->find(C.first.first));
>>> + auto ToDR = dyn_cast_or_null<DefinedRegula
>>> r>(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->Sectio
>>> n);
>>> ----------------
>>> 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/2f032350/attachment-0001.html>
More information about the llvm-commits
mailing list