<div dir="ltr">I'm so sorry that I didn't respond in a timely manner. I'm reading it again now.</div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 27, 2017 at 1:41 PM, Davide Italiano <span dir="ltr"><<a href="mailto:davide@freebsd.org" target="_blank">davide@freebsd.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Rui, any other comments before we push this through the completion line?<br>
As far as I can tell we all agree on the approach taken, and pcc@<br>
wanted to extend this work to handle relocations.<br>
As far as I can tell also Teresa was interested in this work.<br>
Ideally it will be awesome to have this in 6.0 so downstream consumers<br>
can pick it up.<br>
<div><div class="h5"><br>
On Wed, Nov 1, 2017 at 7:07 PM, Michael Spencer via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> On Thu, Oct 26, 2017 at 7:50 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>><br>
> wrote:<br>
>><br>
>> On Tue, Oct 24, 2017 at 9:30 PM, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
>>><br>
>>> Unfortunately, even with --full-shutdown, the result was the same.<br>
>>><br>
>><br>
>> I tested this out and it worked for me with --full-shutdown while linking<br>
>> a trivial object file. One thing I forgot to mention is that you need to<br>
>> also compile the compiler-rt profile runtime at the same version as clang<br>
>> and llvm. It's probably falling back to an incompatible system version.<br>
>><br>
>> - Michael Spencer<br>
><br>
><br>
> I've attached an updated patch based on r317141 that also includes fixes for<br>
> most review comments.<br>
><br>
> - Michael Spencer<br>
><br>
>><br>
>><br>
>>><br>
>>> On Tue, Oct 24, 2017 at 9:25 PM, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
>>>><br>
>>>> On Tue, Oct 24, 2017 at 9:23 PM, Michael Spencer <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>><br>
>>>> wrote:<br>
>>>>><br>
>>>>> On Tue, Oct 24, 2017 at 8:58 PM, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
>>>>>><br>
>>>>>> Sorry, as you pointed out, I made a mistake when I applied the<br>
>>>>>> patches. Now I succeeded to build it. But the output file of llvm-profdata<br>
>>>>>> seems too small. Is this correct?<br>
>>>>>><br>
>>>>>> This is what I did.<br>
>>>>>><br>
>>>>>> $ CC=`which clang` CXX=`which clang++` /ssd/cmake/bin/cmake -GNinja<br>
>>>>>> -DCMAKE_BUILD_TYPE=<wbr>RelWithDebInfo -DLLVM_ENABLE_PROJECTS='clang;<wbr>lld'<br>
>>>>>> -DCMAKE_C{,XX}_FLAGS=-<wbr>fprofile-instr-generate ../llvm-project/llvm/<br>
>>>>>><br>
>>>>>> $ ninja clang lld<br>
>>>>>><br>
>>>>>> $ bin/ld.lld <options-to-link-clang><br>
>>>>>><br>
>>>>>> $ bin/llvm-profdata merge default.profraw -o default.profdata<br>
>>>>>><br>
>>>>>> $ ls -l default.*<br>
>>>>>> -rw-r----- 1 ruiu eng 560 Oct 24 20:55 default.profdata<br>
>>>>>> -rw-r----- 1 ruiu eng 2151240 Oct 24 20:55 default.profraw<br>
>>>>>><br>
>>>>>> $ bin/llvm-profdata show default.profraw<br>
>>>>>> error: default.profraw: Empty raw profile file<br>
>>>>><br>
>>>>><br>
>>>>> It could be that lld is using quick exit and not properly flushing the<br>
>>>>> profile data to disk. I'll test out this configuration.<br>
>>>><br>
>>>><br>
>>>> Ah, it's likely. I'll try again with `--full-shutdown` option.<br>
>>>><br>
>>>>><br>
>>>>> - Michael Spencer<br>
>>>>><br>
>>>>>><br>
>>>>>><br>
>>>>>> On Mon, Oct 23, 2017 at 7:52 PM, Michael Spencer<br>
>>>>>> <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>> wrote:<br>
>>>>>>><br>
>>>>>>> On Mon, Oct 23, 2017 at 6:53 PM, Rui Ueyama <<a href="mailto:ruiu@google.com">ruiu@google.com</a>> wrote:<br>
>>>>>>>><br>
>>>>>>>> It didn't pass cmake because lld/ELF/CMakeLists.txt lacked<br>
>>>>>>>> "CallGraphSort.cpp", so I added that to the CMakeLists.txt.<br>
>>>>>>>><br>
>>>>>>>> It still didn't compile because `Config::CallGraphProfile` doesn't<br>
>>>>>>>> exist, so I added that boolean variable.<br>
>>>>>>>><br>
>>>>>>>> /ssd/llvm-project/lld/ELF/<wbr>CallGraphSort.cpp:274:29: error: no member<br>
>>>>>>>> named 'CallGraphProfile' in 'lld::elf::Configuration'<br>
>>>>>>>> CallGraphSort CGS(Config->CallGraphProfile);<br>
>>>>>>>> ~~~~~~ ^<br>
>>>>>>>><br>
>>>>>>>> Now, I'm seeing the following errors. How can I fix them?<br>
>>>>>>>><br>
>>>>>>>> /ssd/llvm-project/lld/ELF/<wbr>CallGraphSort.cpp:274:17: error: no<br>
>>>>>>>> matching constructor for initialization of '(anonymous<br>
>>>>>>>> namespace)::CallGraphSort'<br>
>>>>>>>> CallGraphSort CGS(Config->CallGraphProfile);<br>
>>>>>>>> ^ ~~~~~~~~~~~~~~~~~~~~~~~~<br>
>>>>>>>> /ssd/llvm-project/lld/ELF/<wbr>CallGraphSort.cpp:43:7: note: candidate<br>
>>>>>>>> constructor (the implicit copy constructor) not viable: no known conversion<br>
>>>>>>>> from 'bool' to 'const (anonymous namespace)::CallGraphSort' for 1st argument<br>
>>>>>>>> class CallGraphSort {<br>
>>>>>>>> ^<br>
>>>>>>>> /ssd/llvm-project/lld/ELF/<wbr>CallGraphSort.cpp:43:7: note: candidate<br>
>>>>>>>> constructor (the implicit move constructor) not viable: no known conversion<br>
>>>>>>>> from 'bool' to '(anonymous namespace)::CallGraphSort' for 1st argument<br>
>>>>>>>> class CallGraphSort {<br>
>>>>>>>> ^<br>
>>>>>>>> /ssd/llvm-project/lld/ELF/<wbr>CallGraphSort.cpp:115:16: note: candidate<br>
>>>>>>>> constructor not viable: no known conversion from 'bool' to<br>
>>>>>>>> 'DenseMap<std::pair<const SymbolBody *, const SymbolBody *>, uint64_t> &'<br>
>>>>>>>> (aka 'DenseMap<pair<const lld::elf::SymbolBody *, const lld::elf::SymbolBody<br>
>>>>>>>> *>, unsigned long> &') for 1st argument<br>
>>>>>>>> CallGraphSort::CallGraphSort(<br>
>>>>>>>> ^<br>
>>>>>>>><br>
>>>>>>><br>
>>>>>>> Are you sure you applied the patch correctly? I just checked the diff<br>
>>>>>>> and it has all that.<br>
>>>>>>><br>
>>>>>>> diff --git a/ELF/CMakeLists.txt b/ELF/CMakeLists.txt<br>
>>>>>>> index 205702975..3b3c388d2 100644<br>
>>>>>>> --- a/ELF/CMakeLists.txt<br>
>>>>>>> +++ b/ELF/CMakeLists.txt<br>
>>>>>>> @@ -18,6 +18,7 @@ add_lld_library(lldELF<br>
>>>>>>> Arch/SPARCV9.cpp<br>
>>>>>>> Arch/X86.cpp<br>
>>>>>>> Arch/X86_64.cpp<br>
>>>>>>> + CallGraphSort.cpp<br>
>>>>>>> Driver.cpp<br>
>>>>>>> DriverUtils.cpp<br>
>>>>>>> EhFrame.cpp<br>
>>>>>>><br>
>>>>>>> It also has the change that adds CallGraphProfile to Config.h.<br>
>>>>>>><br>
>>>>>>> - Michael Spencer<br>
>>>>>>><br>
>>>>>>>><br>
>>>>>>>> On Mon, Oct 23, 2017 at 5:02 PM, Michael Spencer<br>
>>>>>>>> <<a href="mailto:bigcheesegs@gmail.com">bigcheesegs@gmail.com</a>> wrote:<br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>> - Michael Spencer<br>
>>>>>>>>><br>
>>>>>>>>> On Tue, Oct 3, 2017 at 6:43 PM, Rui Ueyama via Phabricator<br>
>>>>>>>>> <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br>
>>>>>>>>>><br>
>>>>>>>>>> ruiu added a comment.<br>
>>>>>>>>>><br>
>>>>>>>>>> Could you send me a patch to produce a call graph file so that I<br>
>>>>>>>>>> can try this patch on my machine?<br>
>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>>> Sorry for the delay. I've attached the llvm and lld patches that<br>
>>>>>>>>> implement a full testable version of the feature.<br>
>>>>>>>>><br>
>>>>>>>>> To use:<br>
>>>>>>>>><br>
>>>>>>>>> -1) Have an elf system with working clang instrumentation based<br>
>>>>>>>>> profiling<br>
>>>>>>>>> 0) Compile clang and lld with the supplied patch<br>
>>>>>>>>> 1) Compile the code with `-fprofile-instr-generate` and link with<br>
>>>>>>>>> any linker<br>
>>>>>>>>> 2) Run the program on a representative sample<br>
>>>>>>>>> 3) `$ llvm-profdata merge default.profraw -o default.profdata`<br>
>>>>>>>>> 4) Compile the code again with<br>
>>>>>>>>> `-fprofile-instr-use=default.<wbr>profdata -ffunction-sections -fuse-ld=lld`<br>
>>>>>>>>><br>
>>>>>>>>> The output of #4 is the program with sections ordered by profile<br>
>>>>>>>>> data. You can add -Wl,-no-call-graph-profile-<wbr>sort to disable sorting to<br>
>>>>>>>>> measure the difference.<br>
>>>>>>>>><br>
>>>>>>>>> `llvm-readobj -elf-cg-profile` will dump the cg profile section.<br>
>>>>>>>>><br>
>>>>>>>>> This also works with LTO.<br>
>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:99-101<br>
>>>>>>>>>> + if (To != Other.To)<br>
>>>>>>>>>> + return To < Other.To;<br>
>>>>>>>>>> + return false;<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> You can just return `To < Other.To`.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:127<br>
>>>>>>>>>> + // Create the graph.<br>
>>>>>>>>>> + for (const auto &C : Profile) {<br>
>>>>>>>>>> + if (C.second == 0)<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> This loop is a bit too dense. It cannot be understood without<br>
>>>>>>>>>> reading each line carefully as I don't understand the whole picture. Please<br>
>>>>>>>>>> insert a blank line between code blocks. Adding more comment would help.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:128<br>
>>>>>>>>>> + for (const auto &C : Profile) {<br>
>>>>>>>>>> + if (C.second == 0)<br>
>>>>>>>>>> + continue;<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> Please define local variables for `C.first.first`,<br>
>>>>>>>>>> `C.first.second` and `C.second` so that they are accessed through meaningful<br>
>>>>>>>>>> names.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:130-135<br>
>>>>>>>>>> + auto FromDR =<br>
>>>>>>>>>> dyn_cast_or_null<<wbr>DefinedRegular>(Symtab->find(<wbr>C.first.first));<br>
>>>>>>>>>> + auto ToDR =<br>
>>>>>>>>>> dyn_cast_or_null<<wbr>DefinedRegular>(Symtab->find(<wbr>C.first.second));<br>
>>>>>>>>>> + if (!FromDR || !ToDR)<br>
>>>>>>>>>> + continue;<br>
>>>>>>>>>> + auto FromSB = dyn_cast_or_null<const<br>
>>>>>>>>>> InputSectionBase>(FromDR-><wbr>Section);<br>
>>>>>>>>>> + auto ToSB = dyn_cast_or_null<const<br>
>>>>>>>>>> InputSectionBase>(ToDR-><wbr>Section);<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> auto -> auto *<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:147<br>
>>>>>>>>>> + Nodes[To].IncidentEdges.push_<wbr>back(EI);<br>
>>>>>>>>>> + } else<br>
>>>>>>>>>> + Edges[EI].Weight = SaturatingAdd(Edges[EI].<wbr>Weight,<br>
>>>>>>>>>> C.second);<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> nit: add {}<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:153<br>
>>>>>>>>>> +<br>
>>>>>>>>>> +void CallGraphSort::contractEdge(<wbr>EdgeIndex CEI) {<br>
>>>>>>>>>> + // Make a copy of the edge as the original will be marked<br>
>>>>>>>>>> killed while being<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> Please add a function comment as to what this function is intended<br>
>>>>>>>>>> to do. I do not understand this function because I don't get a whole<br>
>>>>>>>>>> picture.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:158-163<br>
>>>>>>>>>> + // Remove the self edge from From.<br>
>>>>>>>>>> + FE.erase(std::remove(FE.begin(<wbr>), FE.end(), CEI));<br>
>>>>>>>>>> + std::vector<EdgeIndex> &TE = Nodes[CE.To].IncidentEdges;<br>
>>>>>>>>>> + // Update all edges incident with To to reference From instead.<br>
>>>>>>>>>> Then if they<br>
>>>>>>>>>> + // aren't self edges add them to From.<br>
>>>>>>>>>> + for (EdgeIndex EI : TE) {<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> Add blank lines before comments.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:165-166<br>
>>>>>>>>>> + Edge &E = Edges[EI];<br>
>>>>>>>>>> + // E.From = E.From == CE.To ? CE.From : E.From;<br>
>>>>>>>>>> + // E.To = E.To == CE.To ? CE.From : E.To;<br>
>>>>>>>>>> + if (E.From == CE.To)<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> Please remove debug code.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:178-180<br>
>>>>>>>>>> + // Free memory.<br>
>>>>>>>>>> + std::vector<EdgeIndex>().swap(<wbr>TE);<br>
>>>>>>>>>> +<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> This looks odd. Why do you need to do this? I think you can just<br>
>>>>>>>>>> leave it alone.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:207-208<br>
>>>>>>>>>> +<br>
>>>>>>>>>> +// Group InputSections into clusters using the Call-Chain<br>
>>>>>>>>>> Clustering heuristic<br>
>>>>>>>>>> +// then sort the clusters by density.<br>
>>>>>>>>>> +void CallGraphSort::<wbr>generateClusters() {<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> This might be understood for those who read the paper, but I don't<br>
>>>>>>>>>> think that is enough. Please write more comment as to what are clusters,<br>
>>>>>>>>>> what is density, and what is the heuristic.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/CallGraphSort.cpp:272-273<br>
>>>>>>>>>> +DenseMap<const InputSectionBase *, int><br>
>>>>>>>>>> elf::<wbr>computeCallGraphProfileOrder() {<br>
>>>>>>>>>> + CallGraphSort CGS(Config->CallGraphProfile);<br>
>>>>>>>>>> + return CGS.run();<br>
>>>>>>>>>> +}<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> You can do this in one line without defining a local variable.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> ================<br>
>>>>>>>>>> Comment at: ELF/Writer.cpp:926-928<br>
>>>>>>>>>> + for (BaseCommand *Base : Script->Opt.Commands)<br>
>>>>>>>>>> + if (auto *OS = dyn_cast<OutputSection>(Base))<br>
>>>>>>>>>> + if (OS->Name == ".text") {<br>
>>>>>>>>>> ----------------<br>
>>>>>>>>>> I'd factor this code out as `OutputSection<br>
>>>>>>>>>> *findOutputSection(StringRef Name)`.<br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>> <a href="https://reviews.llvm.org/D36351" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D36351</a><br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>>><br>
>>>>>>>>><br>
>>>>>>>><br>
>>>>>>><br>
>>>>>><br>
>>>>><br>
>>>><br>
>>><br>
>><br>
><br>
><br>
</div></div>> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
><br>
<span class="HOEnZb"><font color="#888888"><br>
--<br>
Davide<br>
<br>
"There are no solved problems; there are only problems that are more<br>
or less solved" -- Henri Poincare<br>
</font></span></blockquote></div><br></div>