<div dir="ltr"><div class="gmail_extra"><div><div class="gmail_signature">On Tue, Oct 24, 2017 at 9:30 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Unfortunately, even with --full-shutdown, the result was the same.</div><div class="gmail-HOEnZb"><div class="gmail-h5"><div class="gmail_extra"><br></div></div></div></blockquote><div><br></div><div>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.</div><div><br></div><div>- Michael Spencer<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5"><div class="gmail_extra"><div class="gmail_quote">On Tue, Oct 24, 2017 at 9:25 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>On Tue, Oct 24, 2017 at 9:23 PM, Michael Spencer <span dir="ltr"><<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail_signature">On Tue, Oct 24, 2017 at 8:58 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br></div></div></span><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">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?<div><br></div><div>This is what I did.<br><div><br></div><div>$ CC=`which clang` CXX=`which clang++` /ssd/cmake/bin/cmake -GNinja -DCMAKE_BUILD_TYPE=RelWithDebI<wbr>nfo -DLLVM_ENABLE_PROJECTS='clang;<wbr>lld' -DCMAKE_C{,XX}_FLAGS=-fprofile<wbr>-instr-generate ../llvm-project/llvm/<br></div><div><br></div><div>$ ninja clang lld</div><div><div><br></div><div>$ bin/ld.lld <options-to-link-clang></div><div><br></div><div>$ bin/llvm-profdata merge default.profraw -o default.profdata<br></div><div><br></div><div>$ ls -l default.*</div><div>-rw-r----- 1 ruiu eng 560 Oct 24 20:55 default.profdata</div><div>-rw-r----- 1 ruiu eng 2151240 Oct 24 20:55 default.profraw</div></div><div><br></div><div><div>$ bin/llvm-profdata show default.profraw </div><div>error: default.profraw: Empty raw profile file</div></div></div></div></blockquote><div><br></div></span><div>It could be that lld is using quick exit and not properly flushing the profile data to disk. I'll test out this configuration.</div></div></div></div></blockquote><div><br></div></span><div>Ah, it's likely. I'll try again with `--full-shutdown` option.</div><div><div class="gmail-m_-1213748041536096030h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div class="gmail-m_-1213748041536096030m_5233438646625426312h5"><div>- Michael Spencer<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Oct 23, 2017 at 7:52 PM, Michael Spencer <span dir="ltr"><<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><span><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail_signature">On Mon, Oct 23, 2017 at 6:53 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br></div></div></span><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">It didn't pass cmake because lld/ELF/CMakeLists.txt lacked "CallGraphSort.cpp", so I added that to the CMakeLists.txt.<div><br></div><div><div>It still didn't compile because `Config::CallGraphProfile` doesn't exist, so I added that boolean variable.</div><div><br></div><div><font face="monospace, monospace">/ssd/llvm-project/lld/ELF/Call<wbr>GraphSort.cpp:274:29: error: no member named 'CallGraphProfile' in 'lld::elf::Configuration'</font></div><div><font face="monospace, monospace"> CallGraphSort CGS(Config->CallGraphProfile);</font></div><div><font face="monospace, monospace"> ~~~~~~ ^</font></div></div><div class="gmail_extra"><br></div><div class="gmail_extra">Now, I'm seeing the following errors. How can I fix them?</div><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_extra"><font face="monospace, monospace">/ssd/llvm-project/lld/ELF/Call<wbr>GraphSort.cpp:274:17: error: no matching constructor for initialization of '(anonymous namespace)::CallGraphSort'</font></div><div class="gmail_extra"><font face="monospace, monospace"> CallGraphSort CGS(Config->CallGraphProfile);</font></div><div class="gmail_extra"><font face="monospace, monospace"> ^ ~~~~~~~~~~~~~~~~~~~~~~~~</font></div><div class="gmail_extra"><font face="monospace, monospace">/ssd/llvm-project/lld/ELF/Call<wbr>GraphSort.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</font></div><div class="gmail_extra"><font face="monospace, monospace">class CallGraphSort {</font></div><div class="gmail_extra"><font face="monospace, monospace"> ^</font></div><div class="gmail_extra"><font face="monospace, monospace">/ssd/llvm-project/lld/ELF/Call<wbr>GraphSort.cpp:43:7: note: candidate constructor (the implicit move constructor) not viable: no known conversion from 'bool' to '(anonymous namespace)::CallGraphSort' for 1st argument</font></div><div class="gmail_extra"><font face="monospace, monospace">class CallGraphSort {</font></div><div class="gmail_extra"><font face="monospace, monospace"> ^</font></div><div class="gmail_extra"><font face="monospace, monospace">/ssd/llvm-project/lld/ELF/Call<wbr>GraphSort.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</font></div><div class="gmail_extra"><font face="monospace, monospace">CallGraphSort::CallGraphSort(</font></div><div class="gmail_extra"><font face="monospace, monospace"> ^</font></div></div><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-h5"><div class="gmail_extra"><br></div></div></div></div></blockquote><div><br></div></span><div>Are you sure you applied the patch correctly? I just checked the diff and it has all that.</div><div><br></div><div><pre class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-aLF-aPX-K0-aPE">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
</pre></div><div>It also has the change that adds CallGraphProfile to Config.h.</div><span class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814HOEnZb"><font color="#888888"><div><br></div><div>- Michael Spencer<br></div></font></span><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814h5"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-h5"><div class="gmail_extra"><div class="gmail_quote">On Mon, Oct 23, 2017 at 5:02 PM, Michael Spencer <span dir="ltr"><<a href="mailto:bigcheesegs@gmail.com" target="_blank">bigcheesegs@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br clear="all"><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-m_935746347903306489gmail_signature">- Michael Spencer</div></div>
<br><div class="gmail_quote"><span class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-">On Tue, Oct 3, 2017 at 6:43 PM, Rui Ueyama via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">ruiu added a comment.<br>
<br>
Could you send me a patch to produce a call graph file so that I can try this patch on my machine?<br></blockquote><div><br></div></span><div>Sorry for the delay. I've attached the llvm and lld patches that implement a full testable version of the feature.</div><div><br></div><div>To use:<br><br></div><div>-1) Have an elf system with working clang instrumentation based profiling</div><div>0) Compile clang and lld with the supplied patch</div><div>1) Compile the code with `-fprofile-instr-generate` and link with any linker</div><div>2) Run the program on a representative sample</div><div>3) `$ llvm-profdata merge default.profraw -o default.profdata`</div><div>4) Compile the code again with `-fprofile-instr-use=default.p<wbr>rofdata -ffunction-sections -fuse-ld=lld`</div><div><br></div><div>The output of #4 is the program with sections ordered by profile data. You can add -Wl,-no-call-graph-profile-sor<wbr>t to disable sorting to measure the difference.</div><div><br></div><div>`llvm-readobj -elf-cg-profile` will dump the cg profile section.</div><div> </div><div>This also works with LTO.</div><div><div class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-h5"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<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>
<span class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-m_935746347903306489gmail-">+ // Create the graph.<br>
</span>+ for (const auto &C : Profile) {<br>
<span class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-m_935746347903306489gmail-">+ if (C.second == 0)<br>
----------------<br>
</span>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.<br>
<br>
<br>
================<br>
Comment at: ELF/CallGraphSort.cpp:128<br>
+ for (const auto &C : Profile) {<br>
<span class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-m_935746347903306489gmail-">+ if (C.second == 0)<br>
</span>+ continue;<br>
----------------<br>
Please define local variables for `C.first.first`, `C.first.second` and `C.second` so that they are accessed through meaningful names.<br>
<br>
<br>
================<br>
Comment at: ELF/CallGraphSort.cpp:130-135<br>
+ auto FromDR = dyn_cast_or_null<DefinedRegula<wbr>r>(Symtab->find(C.first.first)<wbr>);<br>
+ auto ToDR = dyn_cast_or_null<DefinedRegula<wbr>r>(Symtab->find(C.first.second<wbr>));<br>
+ if (!FromDR || !ToDR)<br>
+ continue;<br>
+ auto FromSB = dyn_cast_or_null<const InputSectionBase>(FromDR->Sect<wbr>ion);<br>
+ auto ToSB = dyn_cast_or_null<const InputSectionBase>(ToDR->Sectio<wbr>n);<br>
----------------<br>
auto -> auto *<br>
<br>
<br>
================<br>
Comment at: ELF/CallGraphSort.cpp:147<br>
+ Nodes[To].IncidentEdges.push_b<wbr>ack(EI);<br>
+ } else<br>
+ Edges[EI].Weight = SaturatingAdd(Edges[EI].Weight<wbr>, C.second);<br>
----------------<br>
nit: add {}<br>
<br>
<br>
================<br>
Comment at: ELF/CallGraphSort.cpp:153<br>
+<br>
+void CallGraphSort::contractEdge(Ed<wbr>geIndex CEI) {<br>
+ // Make a copy of the edge as the original will be marked killed while being<br>
----------------<br>
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.<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. 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 leave it alone.<br>
<br>
<br>
================<br>
Comment at: ELF/CallGraphSort.cpp:207-208<br>
+<br>
+// Group InputSections into clusters using the Call-Chain Clustering heuristic<br>
+// then sort the clusters by density.<br>
+void CallGraphSort::generateCluster<wbr>s() {<br>
----------------<br>
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.<br>
<br>
<br>
================<br>
Comment at: ELF/CallGraphSort.cpp:272-273<br>
+DenseMap<const InputSectionBase *, int> elf::computeCallGraphProfileOr<wbr>der() {<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>
<span class="gmail-m_-1213748041536096030m_5233438646625426312m_302232323081226027gmail-m_-1291885123110876243m_-444361670646823264m_7030100685252797814m_4104917064777155823gmail-m_-8064374618096198937gmail-m_935746347903306489gmail-">+ for (BaseCommand *Base : Script->Opt.Commands)<br>
</span>+ if (auto *OS = dyn_cast<OutputSection>(Base))<br>
+ if (OS->Name == ".text") {<br>
----------------<br>
I'd factor this code out as `OutputSection *findOutputSection(StringRef Name)`.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D36351" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3635<wbr>1</a><br>
<br>
<br>
<br>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div></div></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div></div></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div>